diff --git a/README.md b/README.md index cd51aa9..07ecd38 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,8 @@ Ideal for projects using PSR-4, making it easy to reorganize directories without - Additional Extensions: Specify the file extensions to consider during the namespace refactoring process. +- Rename Properties (off by default): When a class is renamed, also rename its class-typed constructor properties (promoted or not, readonly or not) and their `$this->x` usages to match the new class name. + ## Requirements - PHP 7.4+ @@ -47,7 +49,8 @@ This extension contributes the following settings: "php" ], "phpNamespaceRefactor.rename": true, - "phpNamespaceRefactor.editFilesInBackground": true + "phpNamespaceRefactor.editFilesInBackground": true, + "phpNamespaceRefactor.renameProperties": false } ``` @@ -91,6 +94,23 @@ This extension contributes the following settings: - Default: true. +**phpNamespaceRefactor.renameProperties** + +- When a class is renamed, also renames its class-typed constructor properties (promoted or not, readonly or not) and every `$this->x` usage to match the new class name — e.g. `private Test $teste` becomes `private NewTest $newTest` when `Teste` is renamed to `Novo`. +- If more than one property shares the same type in a constructor, the file is skipped rather than guessing which one to rename. +- Accepts either a boolean or an object: + ```json + "phpNamespaceRefactor.renameProperties": true + ``` + ```json + "phpNamespaceRefactor.renameProperties": { + "renameMismatchedNames": true + } + ``` + Setting an object automatically enables the feature. `renameMismatchedNames` (default `false`) additionally renames properties whose current name doesn't already match the class name on purpose (e.g. `private Test $service` becomes `private NewTest $newTest`); without it, only properties already named after the old class are renamed. + +- Default: false. + ## Documentation For architecture, internals, and troubleshooting notes, see [./docs/](./docs/README.md). diff --git a/docs/architecture.md b/docs/architecture.md index c0fe10f..ddc8470 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -16,7 +16,7 @@ Inside `app/`: - `commands/` — entry points triggered by VS Code commands or events (`RenameHandler`, `FileRenameHandler`) - `features/` — orchestrates the flow of a single user interaction (`RenameFeature`) -- `operations/` — runs a full refactor operation (`ClassRenameOperation`, `NamespaceRenameOperation`, `FileMoveOperation`) +- `operations/` — runs a full refactor operation (`ClassRenameOperation`, `NamespaceRenameOperation`, `FileMoveOperation`, `PropertyRenameOperation`) - `services/` — reusable steps used by the operations (`NamespaceBatchUpdater`, `MissingClassImporter`, `DirectoryMovedFilesResolver`, `remove/ImportRemover`, `update/*`) - `subscribers/` — react to workspace events to keep the namespace index up to date (`FileCreatedSubscriber`, `FileDeletedSubscriber`, `FileSavedSubscriber`) diff --git a/docs/configuration.md b/docs/configuration.md index ef05441..1c2ab71 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -12,15 +12,17 @@ All keys are centralized in `ConfigKeys` (`src/domain/workspace/ConfigurationLoc | `phpNamespaceRefactor.additionalExtensions` | `ADDITIONAL_EXTENSIONS` | `string[]` | `["php"]` | `FileExtensionResolver` and `WorkspaceIndex` | | `phpNamespaceRefactor.rename` | `RENAME` | `boolean` | `true` | The `phpNamespaceRefactor.rename` command (`extension.ts`) and the F2 keybinding's `when` clause (`package.json`) | | `phpNamespaceRefactor.editFilesInBackground` | `EDIT_FILES_IN_BACKGROUND` | `boolean` | `true` | `FileEditApplier` | +| `phpNamespaceRefactor.renameProperties` | `RENAME_PROPERTIES` | `boolean \| { renameMismatchedNames?: boolean }` | `false` | `PropertyRenameSettingsResolver`, consumed by `FileMoveOperation`/`PropertyRenameOperation` | ## How configuration is read -Two classes access `workspace.getConfiguration('phpNamespaceRefactor')`, each with a distinct purpose: +Three classes access `workspace.getConfiguration('phpNamespaceRefactor')`, each with a distinct purpose: - **`ConfigurationLocator`** (`src/domain/workspace/ConfigurationLocator.ts`) — generic read, used for settings of any type (`ignoredDirectories`, `additionalExtensions`) -- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`) +- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every plain on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`) +- **`PropertyRenameSettingsResolver`** (`src/domain/property/PropertyRenameSettingsResolver.ts`) — the one setting whose raw value isn't a plain boolean; see [`phpNamespaceRefactor.renameProperties`](#phpnamespacerefactorrenameproperties) below -Neither class caches the `WorkspaceConfiguration` — each instance reads `workspace.getConfiguration()` in its constructor, and since both are `@injectable()` (not singleton), a fresh read happens on every `container.resolve()`. This means a change to the user's configuration is picked up on the next operation, with no need to reload the window. +None of the three caches the `WorkspaceConfiguration` — `ConfigurationLocator`/`FeatureFlagManager` read `workspace.getConfiguration()` in their constructor, and `PropertyRenameSettingsResolver` reads through a fresh `ConfigurationLocator` on every `resolve()` call. All three are `@injectable()` (not singleton), so a fresh read happens on every `container.resolve()`. This means a change to the user's configuration is picked up on the next operation, with no need to reload the window. ## `phpNamespaceRefactor.editFilesInBackground` @@ -38,3 +40,29 @@ Filters files by simple substring match against `fsPath` (not a glob) — see `W ## `phpNamespaceRefactor.additionalExtensions` Normalized by `normalizeExtensions()` (`src/infra/utils/extensions.ts`): strips leading dots and whitespace, lowercases, and always guarantees `php` is included even if not listed. `FileExtensionResolver.match()` sorts extensions from longest to shortest before comparing, so a compound extension (e.g. `class.php`) is never shadowed by the plain `php` entry. + +## `phpNamespaceRefactor.renameProperties` + +Master switch for `PropertyRenameOperation` — off by default, unlike the extension's other flags, because it renames identifiers (constructor parameters, promoted/non-promoted properties, `$this->x` usages), not just type hints. Two conditions gate it: the class itself must have actually been renamed (`oldClassName !== newClassName` — a plain directory move is a no-op), and only files `NamespaceBatchUpdater`/`MultiFileReferenceUpdater` already determined were affected by that specific rename are ever touched, never an independent workspace-wide scan — so an unrelated class in another namespace that happens to share a short name is never at risk. + +For each candidate file, `ClassTypedPropertyLocator` looks for the single constructor property typed as the renamed class (promoted or not, readonly or not); if more than one property shares that type, the file is skipped entirely rather than guessing which one to rename — see [file-move.md](./operations/file-move.md#3-property-rename-propertyrenameoperation-optional). + +### Accepted values + +This one setting doubles as its own sub-option, via `PropertyRenameSettingsResolver`: + +```jsonc +"phpNamespaceRefactor.renameProperties": false // disabled (default) +"phpNamespaceRefactor.renameProperties": true // enabled, mismatched names left alone +"phpNamespaceRefactor.renameProperties": {} // enabled, mismatched names left alone +"phpNamespaceRefactor.renameProperties": { "renameMismatchedNames": true } // enabled, mismatched names renamed too +``` + +`renameMismatchedNames` controls what happens when a property's current name doesn't already follow the class-name convention: + +- unset/`false` (default) — a property is only renamed if its name already matches the *old* class name (e.g. `$teste` for `Teste`); a property named something else on purpose (e.g. `$service`) is left untouched +- `true` — mismatched names are renamed too, to match the *new* class name (e.g. `$service` → `$novo`) + +Any object value implies the feature is enabled — `false` is the only way to turn it off; there's no `{ "enabled": false }` form. + +**Why one polymorphic setting instead of two plain booleans:** VS Code's settings schema doesn't allow a key to be both a leaf value and the parent of another key. An earlier version declared `phpNamespaceRefactor.renameProperties` (boolean) alongside `phpNamespaceRefactor.renameProperties.renameMismatchedNames` (boolean) as two separate keys — VS Code detected the conflict, logged `Ignoring phpNamespaceRefactor.renameProperties.renameMismatchedNames as phpNamespaceRefactor.renameProperties is false` in the console, and silently resolved **both** settings to `false` regardless of what the user configured. Collapsing them into a single `boolean | object` setting sidesteps the conflict entirely, since there's only ever one registered key. diff --git a/docs/operations/class-rename.md b/docs/operations/class-rename.md index 42068a9..d0b3afa 100644 --- a/docs/operations/class-rename.md +++ b/docs/operations/class-rename.md @@ -31,6 +31,7 @@ It then delegates the rename to `FileRenameHandler.create()`, which triggers VS - Update the `namespace` declaration in the file - Update the class name inside the file (via `ClassNameUpdater`) - Update every `use` statement referencing the class throughout the project +- Optionally, rename class-typed constructor properties (and their `$this->x` usages) in the affected files to match the new class name — only when `renameProperties` is enabled, see `PropertyRenameOperation` in [file-move.md](./file-move.md#3-property-rename-propertyrenameoperation-optional) ## Difference from a direct Explorer rename diff --git a/docs/operations/file-move.md b/docs/operations/file-move.md index c87173a..b139f11 100644 --- a/docs/operations/file-move.md +++ b/docs/operations/file-move.md @@ -15,8 +15,9 @@ onDidRenameFiles (VS Code event) → DirectoryMovedFilesResolver.execute() 1. expands directory moves into per-file moves → for each .php file: → NamespaceBatchUpdater.execute() 2. updates namespace/class + references - → MissingClassImporter.execute() 3. (optional) auto-imports classes from the old directory - → ImportRemover.execute() 4. (optional, checked internally) removes stale imports + → PropertyRenameOperation.execute() 3. (optional) renames class-typed properties to match the new class name + → MissingClassImporter.execute() 4. (optional) auto-imports classes from the old directory + → ImportRemover.execute() 5. (optional, checked internally) removes stale imports ``` ### Serialized queue @@ -46,6 +47,8 @@ For each moved file, `src/app/services/NamespaceBatchUpdater.ts`: 5. If the namespace declaration wasn't found/changed (step 4 returns `false`), the operation stops here — there's nothing to propagate for a file with no `namespace` declaration 6. Otherwise, calls `MultiFileReferenceUpdater` to propagate the change across the rest of the project +`NamespaceBatchUpdater.execute()` returns the list of files `MultiFileReferenceUpdater` determined were affected (empty when it never ran, e.g. no `namespace` declaration to key off). `FileMoveOperation` forwards that same list into step 3 below, so property renaming only ever touches files this specific rename actually reached — never an independent, broader scan. + #### `MultiFileReferenceUpdater` — how affected files are found Combines two sources, without relying on either alone: @@ -62,13 +65,24 @@ For each affected file, it replaces via regex: Every edit across every affected file is accumulated into a **single `WorkspaceEdit`** before being applied (`FileEditApplier.apply`), so the whole refactor shows up as one undo-stack entry in VS Code instead of one edit per file — see [issue #72](https://github.com/rejmann/php-namespace-refactor/issues/72). -At the end, `MultiFileReferenceUpdater` always calls `ImportRemover.execute({ uri: newUri })` for the moved file itself (in addition to the call `FileMoveOperation` already makes in step 4 below — both are no-ops if the flag is disabled or there's nothing to remove). +At the end, `MultiFileReferenceUpdater` always calls `ImportRemover.execute({ uri: newUri })` for the moved file itself (in addition to the call `FileMoveOperation` already makes in step 5 below — both are no-ops if the flag is disabled or there's nothing to remove). + +### 3. Property rename (`PropertyRenameOperation`, optional) + +Only runs if `renameProperties` resolves to enabled (off by default — see [configuration.md](../configuration.md#phpnamespacerefactorrenameproperties)). `PropertyRenameSettingsResolver` reads the raw setting (`boolean | { renameMismatchedNames?: boolean }`) once in `FileMoveOperation` and turns it into `{ enabled, renameMismatchedNames }`; only `enabled` gates whether this step runs at all. Also only acts when the class itself was actually renamed (`oldClassName !== newClassName`); a plain move to a different directory with the same class name is a no-op. + +1. Derives the expected old/new property names from the class names (`PropertyNameResolver`, e.g. `Teste` → `teste`) +2. Builds the candidate file list from the `affectedFiles` `FileMoveOperation` received back from `NamespaceBatchUpdater` (step 2's output), plus the moved file itself — deliberately **not** an independent workspace-wide scan, so a differently-namespaced class that happens to share a short name is never touched +3. For each candidate file whose text contains the new class name, `ClassTypedPropertyLocator` looks for the constructor property typed as that class — promoted or not, readonly or not, confirming a non-promoted parameter by checking for a `$this->x = $x;` assignment in the constructor body (see `ConstructorSpanFinder` for how the constructor's parameter list and body are located via brace/paren matching). The property's own declaration line doesn't need a type hint to be found this way — a legacy `private $x;` typed only via a `@var ClassName` docblock is still matched and renamed, via `PropertyDeclarationPattern`, once the constructor param already confirmed what class it holds +4. If more than one property shares that type in the same file, it's ambiguous — the file is skipped entirely rather than guessing +5. If exactly one property is found: it's renamed when its current name already matches the *old* class-name convention, or — only when `renameMismatchedNames` also resolved to `true` — regardless of what it was named before +6. Renaming rewrites the constructor parameter/property declaration and every `$this->x` usage in that file, accumulated into a single `WorkspaceEdit` (its own undo stop, separate from `MultiFileReferenceUpdater`'s) -### 3. Auto-import of classes from the source directory (`MissingClassImporter`, optional) +### 4. Auto-import of classes from the source directory (`MissingClassImporter`, optional) Only runs if the `autoImportNamespace` flag is enabled. Lists the `.php` files still left in the source directory, checks which classes from those files are used in the moved file's text but not imported, and inserts the corresponding `use` statements. -### 4. Removing stale imports (`ImportRemover`) +### 5. Removing stale imports (`ImportRemover`) Unlike the other flags, the `removeUnusedImports` check happens **inside** `ImportRemover` itself (not in `FileMoveOperation`) — so it's always called, but returns immediately if the flag is disabled. @@ -78,8 +92,10 @@ When enabled: it collects the class names declared in the other files of the mov | Flag | Behavior | |---|---| -| `autoImportNamespace` | Enables step 3 (auto-import of classes from the old directory) | -| `removeUnusedImports` | Enables the import removal in step 4 (checked inside `ImportRemover`) | +| `renameProperties` (`true`/`{}`) | Enables step 3 (renaming class-typed constructor properties to match the new class name) | +| `renameProperties: { renameMismatchedNames: true }` | Extends step 3 to also rename properties whose name doesn't already match the class-name convention | +| `autoImportNamespace` | Enables step 4 (auto-import of classes from the old directory) | +| `removeUnusedImports` | Enables the import removal in step 5 (checked inside `ImportRemover`) | | `editFilesInBackground` | Doesn't change what's edited, only whether touched files open a tab in the editor or are saved silently — see [configuration.md](../configuration.md) | ## Error handling @@ -90,6 +106,7 @@ Each file in the batch is processed inside its own `try/catch` in `FileMoveOpera - `DirectoryMovedFilesResolver` — expands directory moves into per-file moves - `NamespaceBatchUpdater` — orchestrates the namespace, class name, and reference update +- `PropertyRenameOperation` — renames class-typed constructor properties (and their `$this->x` usages) to match a renamed class - `MissingClassImporter` — detects and injects missing imports - `ImportRemover` — removes unused imports - `FeatureFlagManager` — checks which features are enabled in the settings diff --git a/package.json b/package.json index 2858e79..1182fd3 100644 --- a/package.json +++ b/package.json @@ -109,6 +109,19 @@ "type": "boolean", "default": true, "description": "Apply refactor edits to files without opening them, keeping only files that were already open in the editor. Disable to have every edited file opened in the editor as before." + }, + "phpNamespaceRefactor.renameProperties": { + "type": ["boolean", "object"], + "default": false, + "properties": { + "renameMismatchedNames": { + "type": "boolean", + "default": false, + "description": "Also rename properties whose current name doesn't match the class name (e.g. $service for a Teste type)." + } + }, + "additionalProperties": false, + "description": "Rename constructor-typed properties (promoted or not, readonly or not) - and their $this->x usages - to match the class name when renaming a class. Set to true/false to toggle, or to an object like { \"renameMismatchedNames\": true } to also rename properties whose current name doesn't match the class." } } } diff --git a/src/app/operations/FileMoveOperation.ts b/src/app/operations/FileMoveOperation.ts index 11562aa..6418aaa 100644 --- a/src/app/operations/FileMoveOperation.ts +++ b/src/app/operations/FileMoveOperation.ts @@ -2,11 +2,13 @@ import { DirectoryMovedFilesResolver } from '@app/services/DirectoryMovedFilesRe import { MissingClassImporter } from '@app/services/MissingClassImporter'; import { NamespaceBatchUpdater } from '@app/services/NamespaceBatchUpdater'; import { ImportRemover } from '@app/services/remove/ImportRemover'; +import { PropertyRenameSettingsResolver } from '@domain/property/PropertyRenameSettingsResolver'; import { ConfigKeys } from '@domain/workspace/ConfigurationLocator'; import { FeatureFlagManager } from '@domain/workspace/FeatureFlagManager'; import { inject, injectable } from 'tsyringe'; import type { FileMove } from './FileMove'; +import { PropertyRenameOperation } from './PropertyRenameOperation'; @injectable() export class FileMoveOperation { @@ -16,6 +18,8 @@ export class FileMoveOperation { @inject(MissingClassImporter) private missingClassImporter: MissingClassImporter, @inject(ImportRemover) private importRemover: ImportRemover, @inject(FeatureFlagManager) private featureFlagManager: FeatureFlagManager, + @inject(PropertyRenameOperation) private propertyRenameOperation: PropertyRenameOperation, + @inject(PropertyRenameSettingsResolver) private propertyRenameSettingsResolver: PropertyRenameSettingsResolver, ) {} public async execute(files: ReadonlyArray): Promise { @@ -27,7 +31,17 @@ export class FileMoveOperation { } try { - await this.namespaceBatchUpdater.execute({ newUri, oldUri }); + const affectedFiles = await this.namespaceBatchUpdater.execute({ newUri, oldUri }); + + const propertyRenameSettings = this.propertyRenameSettingsResolver.resolve(); + if (propertyRenameSettings.enabled) { + await this.propertyRenameOperation.execute({ + oldUri, + newUri, + affectedFiles, + renameMismatchedNames: propertyRenameSettings.renameMismatchedNames, + }); + } if (this.featureFlagManager.isActive({ key: ConfigKeys.AUTO_IMPORT_NAMESPACE })) { await this.missingClassImporter.execute({ oldUri, newUri }); diff --git a/src/app/operations/PropertyRenameOperation.ts b/src/app/operations/PropertyRenameOperation.ts new file mode 100644 index 0000000..654baa6 --- /dev/null +++ b/src/app/operations/PropertyRenameOperation.ts @@ -0,0 +1,156 @@ +import { ClassTypedPropertyLocator, PropertyMatch } from '@domain/property/ClassTypedPropertyLocator'; +import { ConstructorSpanFinder } from '@domain/property/ConstructorSpanFinder'; +import { buildPropertyDeclarationPattern } from '@domain/property/PropertyDeclarationPattern'; +import { PropertyNameResolver } from '@domain/property/PropertyNameResolver'; +import { WorkspacePathResolver } from '@domain/workspace/WorkspacePathResolver'; +import { FileEditApplier } from '@infra/vscode/FileEditApplier'; +import { TextDocumentOpener } from '@infra/vscode/TextDocumentOpener'; +import { inject, injectable } from 'tsyringe'; +import { Range, TextDocument, Uri, WorkspaceEdit } from 'vscode'; + +interface Props { + oldUri: Uri + newUri: Uri + affectedFiles: Uri[] + renameMismatchedNames: boolean +} + +@injectable() +export class PropertyRenameOperation { + constructor( + @inject(WorkspacePathResolver) private workspacePathResolver: WorkspacePathResolver, + @inject(TextDocumentOpener) private textDocumentOpener: TextDocumentOpener, + @inject(FileEditApplier) private fileEditApplier: FileEditApplier, + @inject(ClassTypedPropertyLocator) private classTypedPropertyLocator: ClassTypedPropertyLocator, + @inject(PropertyNameResolver) private propertyNameResolver: PropertyNameResolver, + @inject(ConstructorSpanFinder) private constructorSpanFinder: ConstructorSpanFinder, + ) {} + + public async execute({ oldUri, newUri, affectedFiles, renameMismatchedNames }: Props): Promise { + const oldClassName = this.workspacePathResolver.extractClassNameFromPath(oldUri.fsPath); + const newClassName = this.workspacePathResolver.extractClassNameFromPath(newUri.fsPath); + + if (!oldClassName || !newClassName || oldClassName === newClassName) { + return; + } + + const expectedOldName = this.propertyNameResolver.resolve(oldClassName); + const expectedNewName = this.propertyNameResolver.resolve(newClassName); + + const files = this.getCandidateFiles(newUri, affectedFiles); + const edit = new WorkspaceEdit(); + + await Promise.all(files.map(async (file) => { + try { + const { document, text } = await this.textDocumentOpener.execute({ uri: file }); + if (!text.includes(newClassName)) { + return; + } + + const match = this.classTypedPropertyLocator.execute({ text, className: newClassName }); + if (!match || match.propertyName === expectedNewName) { + return; + } + + const matchesOldConvention = match.propertyName === expectedOldName; + if (!matchesOldConvention && !renameMismatchedNames) { + return; + } + + this.addPropertyRenameEdits(edit, file, document, text, newClassName, match, expectedNewName); + } catch (_) { + return; + } + })); + + await this.fileEditApplier.apply(edit); + } + + private addPropertyRenameEdits( + edit: WorkspaceEdit, + uri: Uri, + document: TextDocument, + text: string, + className: string, + match: PropertyMatch, + newName: string, + ): void { + const oldName = match.propertyName; + + const variableSpans = this.buildVariableRenameSpans(text, className, match); + for (const [start, end] of variableSpans) { + this.replaceInRange(edit, uri, document, text, start, end, new RegExp(`\\$${oldName}\\b`, 'g'), `$${newName}`); + } + + this.replaceInRange( + edit, uri, document, text, 0, text.length, + new RegExp(`\\$this->${oldName}\\b`, 'g'), `$this->${newName}`, + ); + } + + private buildVariableRenameSpans(text: string, className: string, match: PropertyMatch): [number, number][] { + const spans: [number, number][] = []; + + const constructorSpan = this.findConstructorSpan(text); + if (constructorSpan) { + spans.push(constructorSpan); + } + + if (match.hasSeparateDeclaration) { + const declarationSpan = this.findDeclarationSpan(text, className, match.propertyName); + if (declarationSpan) { + spans.push(declarationSpan); + } + } + + return spans; + } + + private findConstructorSpan(text: string): [number, number] | null { + const span = this.constructorSpanFinder.find(text); + return span ? [span.constructorStart, span.bodyEnd] : null; + } + + private findDeclarationSpan(text: string, className: string, propertyName: string): [number, number] | null { + const match = buildPropertyDeclarationPattern(className, propertyName).exec(text); + return match ? [match.index, match.index + match[0].length] : null; + } + + /** + * Only the files MultiFileReferenceUpdater already determined were + * affected by this exact class rename (plus the renamed file itself) - + * never a broader workspace scan, so a differently-namespaced class that + * happens to share a short name is never touched. + */ + private getCandidateFiles(newUri: Uri, affectedFiles: Uri[]): Uri[] { + const files = [newUri, ...affectedFiles]; + const seen = new Set(); + + return files.filter((file) => { + if (seen.has(file.fsPath)) { + return false; + } + seen.add(file.fsPath); + return true; + }); + } + + private replaceInRange( + edit: WorkspaceEdit, + uri: Uri, + document: TextDocument, + text: string, + rangeStart: number, + rangeEnd: number, + regex: RegExp, + replacement: string, + ): void { + const scoped = text.slice(rangeStart, rangeEnd); + + for (const match of scoped.matchAll(regex)) { + const start = rangeStart + (match.index as number); + const end = start + match[0].length; + edit.replace(uri, new Range(document.positionAt(start), document.positionAt(end)), replacement); + } + } +} diff --git a/src/app/services/NamespaceBatchUpdater.ts b/src/app/services/NamespaceBatchUpdater.ts index efddd27..4c3a5b2 100644 --- a/src/app/services/NamespaceBatchUpdater.ts +++ b/src/app/services/NamespaceBatchUpdater.ts @@ -20,11 +20,11 @@ export class NamespaceBatchUpdater { @inject(ClassNameUpdater) private classNameUpdater: ClassNameUpdater, ) {} - public async execute({ newUri, oldUri }: Props) { + public async execute({ newUri, oldUri }: Props): Promise { const { namespace, fullNamespace } = await this.getNamespace(newUri); if (!namespace) { - return; + return []; } const { namespace: old, fullNamespace: oldFullNamespace } = await this.getNamespace(oldUri); @@ -39,10 +39,10 @@ export class NamespaceBatchUpdater { }); if (!isUpdated) { - return; + return []; } - await this.multiFileReferenceUpdater.execute({ + return await this.multiFileReferenceUpdater.execute({ useOldNamespace: oldFullNamespace, useNewNamespace: fullNamespace, newUri, diff --git a/src/app/services/update/ClassNameUpdater.ts b/src/app/services/update/ClassNameUpdater.ts index 9a89761..9f888a3 100644 --- a/src/app/services/update/ClassNameUpdater.ts +++ b/src/app/services/update/ClassNameUpdater.ts @@ -1,3 +1,4 @@ +import { ClassNameBoundaryRegexBuilder } from '@domain/namespace/ClassNameBoundaryRegexBuilder'; import { PHP_CLASS_DECLARATION_REGEX } from '@domain/namespace/PhpPatterns'; import { WorkspacePathResolver } from '@domain/workspace/WorkspacePathResolver'; import { FileEditApplier } from '@infra/vscode/FileEditApplier'; @@ -15,6 +16,7 @@ export class ClassNameUpdater { @inject(TextDocumentOpener) private textDocumentOpener: TextDocumentOpener, @inject(WorkspacePathResolver) private workspacePathResolver: WorkspacePathResolver, @inject(FileEditApplier) private fileEditApplier: FileEditApplier, + @inject(ClassNameBoundaryRegexBuilder) private classNameBoundaryRegexBuilder: ClassNameBoundaryRegexBuilder, ) {} public async execute({ newUri }: Props): Promise { @@ -32,7 +34,10 @@ export class ClassNameUpdater { return; } - const newText = text.replace(new RegExp(`\\b${currentName}\\b`, 'g'), expectedName); + const newText = text.replace( + this.classNameBoundaryRegexBuilder.execute({ className: currentName }), + expectedName, + ); const edit = new WorkspaceEdit(); edit.replace( diff --git a/src/app/services/update/MultiFileReferenceUpdater.ts b/src/app/services/update/MultiFileReferenceUpdater.ts index f24ab9d..6ae94a2 100644 --- a/src/app/services/update/MultiFileReferenceUpdater.ts +++ b/src/app/services/update/MultiFileReferenceUpdater.ts @@ -1,4 +1,6 @@ import { ImportRemover } from '@app/services/remove/ImportRemover'; +import { ClassNameBoundaryRegexBuilder } from '@domain/namespace/ClassNameBoundaryRegexBuilder'; +import { NOT_FOLLOWED_BY_NAMESPACE_CHAR } from '@domain/namespace/PhpPatterns'; import { UseStatementCreator } from '@domain/namespace/UseStatementCreator'; import { UseStatementInjector } from '@domain/namespace/UseStatementInjector'; import { UseStatementLocator } from '@domain/namespace/UseStatementLocator'; @@ -34,6 +36,7 @@ export class MultiFileReferenceUpdater { @inject(UseStatementLocator) private useStatementLocator: UseStatementLocator, @inject(UseStatementInjector) private useStatementInjector: UseStatementInjector, @inject(FileEditApplier) private fileEditApplier: FileEditApplier, + @inject(ClassNameBoundaryRegexBuilder) private classNameBoundaryRegexBuilder: ClassNameBoundaryRegexBuilder, ) {} public async execute({ @@ -41,16 +44,16 @@ export class MultiFileReferenceUpdater { useNewNamespace, newUri, oldUri, - }: Props) { + }: Props): Promise { const directoryPath = this.workspacePathResolver.extractDirectoryFromPath(oldUri.fsPath); const className = this.workspacePathResolver.extractClassNameFromPath(oldUri.fsPath); const newClassName = this.workspacePathResolver.extractClassNameFromPath(newUri.fsPath); const useImport = this.useStatementCreator.single({ fullNamespace: useNewNamespace }); const ignoreFile = newUri.fsPath; - const namespaceRegex = new RegExp(this.escapeRegex(useOldNamespace), 'g'); + const namespaceRegex = new RegExp(`${this.escapeRegex(useOldNamespace)}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'); const classNameRegex = className !== newClassName - ? new RegExp(`\\b${className}\\b`, 'g') + ? this.classNameBoundaryRegexBuilder.execute({ className }) : null; // Files that import/use the old namespace. @@ -117,6 +120,8 @@ export class MultiFileReferenceUpdater { await this.fileEditApplier.apply(edit); await this.importRemover.execute({ uri: newUri }); + + return [...affectedPaths.map(fsPath => Uri.file(fsPath)), ...sameDirectoryFiles]; } /** diff --git a/src/domain/namespace/ClassNameBoundaryRegexBuilder.ts b/src/domain/namespace/ClassNameBoundaryRegexBuilder.ts new file mode 100644 index 0000000..a038876 --- /dev/null +++ b/src/domain/namespace/ClassNameBoundaryRegexBuilder.ts @@ -0,0 +1,14 @@ +import { injectable } from 'tsyringe'; + +import { NOT_FOLLOWED_BY_NAMESPACE_CHAR, NOT_PRECEDED_BY_NAMESPACE_CHAR } from './PhpPatterns'; + +interface Props { + className: string, +} + +@injectable() +export class ClassNameBoundaryRegexBuilder { + public execute({ className }: Props): RegExp { + return new RegExp(`${NOT_PRECEDED_BY_NAMESPACE_CHAR}${className}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'); + } +} diff --git a/src/domain/namespace/PhpPatterns.ts b/src/domain/namespace/PhpPatterns.ts index bf58b29..8b5ed01 100644 --- a/src/domain/namespace/PhpPatterns.ts +++ b/src/domain/namespace/PhpPatterns.ts @@ -20,3 +20,11 @@ export const PHP_CLASS_DECLARATION_REGEX = new RegExp( `^\\s*${DECLARATION_MODIFIER_PATTERN}(?:${NAMED_TYPE_PATTERN})\\s+(\\w+)`, 'm' ); + +// A namespace/identifier boundary: neither an identifier character (letter, +// digit, underscore) nor a namespace separator (\) can sit on this side of a +// match, otherwise the match is actually a prefix or suffix of a longer FQCN +// rather than the identifier itself — e.g. matching "Foo" inside "FooBar" or +// inside "Foo\Bar\Baz" (a sub-namespace that merely starts with "Foo"). +export const NOT_PRECEDED_BY_NAMESPACE_CHAR = '(? { - const regex = new RegExp(`\\b${className}\\b`, 'g'); + const regex = this.classNameBoundaryRegexBuilder.execute({ className }); if (regex.test(contentDocument) && !classesUsed.includes(className)) { classesUsed.push(className); } diff --git a/src/domain/property/ClassTypedPropertyLocator.ts b/src/domain/property/ClassTypedPropertyLocator.ts new file mode 100644 index 0000000..0b39828 --- /dev/null +++ b/src/domain/property/ClassTypedPropertyLocator.ts @@ -0,0 +1,124 @@ +import { inject, injectable } from 'tsyringe'; + +import { ConstructorSpanFinder } from './ConstructorSpanFinder'; +import { buildPropertyDeclarationPattern } from './PropertyDeclarationPattern'; + +const VISIBILITY = 'public|protected|private'; + +interface Props { + text: string + className: string +} + +export interface PropertyMatch { + propertyName: string + isPromoted: boolean + hasSeparateDeclaration: boolean +} + +/** + * Locates the single constructor property that represents an instance of + * `className` in a PHP file's source text - promoted or not, readonly or + * not. Returns null when there's no such property, or when more than one + * parameter shares that type (renaming either would be a guess that risks + * a variable-name collision). + */ +@injectable() +export class ClassTypedPropertyLocator { + constructor( + @inject(ConstructorSpanFinder) private constructorSpanFinder: ConstructorSpanFinder, + ) {} + + public execute({ text, className }: Props): PropertyMatch | null { + const span = this.constructorSpanFinder.find(text); + if (!span) { + return null; + } + + const params = text.slice(span.paramsStart, span.paramsEnd); + const body = text.slice(span.bodyStart, span.bodyEnd); + + const candidates = this.splitParams(params) + .map(param => this.matchParam(param, body, text, className)) + .filter((match): match is PropertyMatch => match !== null); + + return candidates.length === 1 ? candidates[0] : null; + } + + private hasPropertyDeclaration(text: string, className: string, varName: string): boolean { + return buildPropertyDeclarationPattern(className, varName).test(text); + } + + private isAssignedToThis(constructorBody: string, varName: string): boolean { + const pattern = new RegExp(`\\$this->${varName}\\s*=\\s*\\$${varName}\\s*;`); + return pattern.test(constructorBody); + } + + private matchParam(param: string, constructorBody: string, text: string, className: string): PropertyMatch | null { + const cleanedParam = this.stripAttributes(param); + + const promotedName = this.matchPromoted(cleanedParam, className); + if (promotedName) { + return { propertyName: promotedName, isPromoted: true, hasSeparateDeclaration: false }; + } + + const plainName = this.matchPlain(cleanedParam, className); + if (!plainName || !this.isAssignedToThis(constructorBody, plainName)) { + return null; + } + + return { + propertyName: plainName, + isPromoted: false, + hasSeparateDeclaration: this.hasPropertyDeclaration(text, className, plainName), + }; + } + + private matchPlain(cleanedParam: string, className: string): string | null { + if (new RegExp(`\\b(?:${VISIBILITY}|readonly)\\b`).test(cleanedParam)) { + return null; + } + + const pattern = new RegExp(`\\??\\b${className}\\b\\s+\\$(\\w+)`); + return pattern.exec(cleanedParam)?.[1] ?? null; + } + + private matchPromoted(cleanedParam: string, className: string): string | null { + const pattern = new RegExp( + `(?:(?:${VISIBILITY})\\s+(?:readonly\\s+)?|readonly\\s+(?:${VISIBILITY})\\s+)\\??\\b${className}\\b\\s+\\$(\\w+)`, + ); + return pattern.exec(cleanedParam)?.[1] ?? null; + } + + private splitParams(params: string): string[] { + const result: string[] = []; + let depth = 0; + let current = ''; + + for (const char of params) { + if (char === '(' || char === '[' || char === '{') { + depth++; + } else if (char === ')' || char === ']' || char === '}') { + depth--; + } + + if (char === ',' && depth === 0) { + result.push(current); + current = ''; + continue; + } + + current += char; + } + + if (current.trim()) { + result.push(current); + } + + return result; + } + + private stripAttributes(param: string): string { + return param.replace(/#\[[^\]]*\]/g, ' ').trim(); + } +} diff --git a/src/domain/property/ConstructorSpanFinder.ts b/src/domain/property/ConstructorSpanFinder.ts new file mode 100644 index 0000000..256e6dd --- /dev/null +++ b/src/domain/property/ConstructorSpanFinder.ts @@ -0,0 +1,61 @@ +import { injectable } from 'tsyringe'; + +export interface ConstructorSpan { + constructorStart: number + paramsStart: number + paramsEnd: number + bodyStart: number + bodyEnd: number +} + +@injectable() +export class ConstructorSpanFinder { + public find(text: string): ConstructorSpan | null { + const signatureMatch = /function\s+__construct\s*\(/.exec(text); + if (!signatureMatch) { + return null; + } + + const paramsStart = signatureMatch.index + signatureMatch[0].length; + const paramsEnd = this.findMatching(text, paramsStart - 1, '(', ')'); + if (paramsEnd === -1) { + return null; + } + + const bodyStart = text.indexOf('{', paramsEnd); + if (bodyStart === -1) { + return { + constructorStart: signatureMatch.index, + paramsStart, + paramsEnd, + bodyStart: paramsEnd + 1, + bodyEnd: paramsEnd + 1, + }; + } + + const bodyEnd = this.findMatching(text, bodyStart, '{', '}'); + + return { + constructorStart: signatureMatch.index, + paramsStart, + paramsEnd, + bodyStart, + bodyEnd: bodyEnd === -1 ? text.length : bodyEnd + 1, + }; + } + + public findMatching(text: string, openIndex: number, open: string, close: string): number { + let depth = 0; + for (let i = openIndex; i < text.length; i++) { + if (text[i] === open) { + depth++; + } else if (text[i] === close) { + depth--; + if (depth === 0) { + return i; + } + } + } + return -1; + } +} diff --git a/src/domain/property/PropertyDeclarationPattern.ts b/src/domain/property/PropertyDeclarationPattern.ts new file mode 100644 index 0000000..f03454d --- /dev/null +++ b/src/domain/property/PropertyDeclarationPattern.ts @@ -0,0 +1,16 @@ +const VISIBILITY = 'public|protected|private'; + +/** + * Matches a class-body property declaration for `varName` - e.g. + * `private Test $teste;` or, since the type hint is optional in PHP, + * a legacy `private $teste;` typed only via a `@var Teste` docblock. + * `className` is accepted but not required, so a property whose type was + * never declared in code (only documented) is still found once the caller + * has already confirmed by other means (e.g. a constructor assignment) + * that it holds an instance of that class. + */ +export function buildPropertyDeclarationPattern(className: string, varName: string): RegExp { + return new RegExp( + `(?:${VISIBILITY})\\s+(?:readonly\\s+)?(?:\\??\\b${className}\\b\\s+)?\\$${varName}\\s*;`, + ); +} diff --git a/src/domain/property/PropertyNameResolver.ts b/src/domain/property/PropertyNameResolver.ts new file mode 100644 index 0000000..fe75a44 --- /dev/null +++ b/src/domain/property/PropertyNameResolver.ts @@ -0,0 +1,8 @@ +import { injectable } from 'tsyringe'; + +@injectable() +export class PropertyNameResolver { + public resolve(className: string): string { + return className.charAt(0).toLowerCase() + className.slice(1); + } +} diff --git a/src/domain/property/PropertyRenameSettingsResolver.ts b/src/domain/property/PropertyRenameSettingsResolver.ts new file mode 100644 index 0000000..3d04c79 --- /dev/null +++ b/src/domain/property/PropertyRenameSettingsResolver.ts @@ -0,0 +1,35 @@ +import { ConfigKeys, ConfigurationLocator } from '@domain/workspace/ConfigurationLocator'; +import { inject, injectable } from 'tsyringe'; + +export interface PropertyRenameSettings { + enabled: boolean + renameMismatchedNames: boolean +} + +type RenamePropertiesValue = boolean | { renameMismatchedNames?: boolean }; + +/** + * `phpNamespaceRefactor.renameProperties` is a single setting that accepts + * either a boolean or an object (`{ renameMismatchedNames: boolean }`) - + * any object value implies the feature is enabled, since a bare boolean + * `false` is the only way to turn it off. + */ +@injectable() +export class PropertyRenameSettingsResolver { + constructor( + @inject(ConfigurationLocator) private configurationLocator: ConfigurationLocator, + ) {} + + public resolve(): PropertyRenameSettings { + const value = this.configurationLocator.get({ + key: ConfigKeys.RENAME_PROPERTIES, + defaultValue: false, + }); + + if (typeof value === 'object' && value !== null) { + return { enabled: true, renameMismatchedNames: value.renameMismatchedNames === true }; + } + + return { enabled: value === true, renameMismatchedNames: false }; + } +} diff --git a/src/domain/workspace/ConfigurationLocator.ts b/src/domain/workspace/ConfigurationLocator.ts index e0d1e7c..78010c3 100644 --- a/src/domain/workspace/ConfigurationLocator.ts +++ b/src/domain/workspace/ConfigurationLocator.ts @@ -10,6 +10,10 @@ export const ConfigKeys = { ADDITIONAL_EXTENSIONS: 'additionalExtensions', RENAME: 'rename', EDIT_FILES_IN_BACKGROUND: 'editFilesInBackground', + // Value is boolean|object (see PropertyRenameSettingsResolver) rather than a plain + // boolean - a single polymorphic key avoids VS Code's settings schema conflict that + // comes from one key being both a leaf boolean and the parent of another setting. + RENAME_PROPERTIES: 'renameProperties', } as const; export type Props = { diff --git a/src/test/ClassNameUpdater.test.ts b/src/test/ClassNameUpdater.test.ts index fd59cca..3f059d1 100644 --- a/src/test/ClassNameUpdater.test.ts +++ b/src/test/ClassNameUpdater.test.ts @@ -7,6 +7,7 @@ import * as path from 'path'; import * as vscode from 'vscode'; import { ClassNameUpdater } from '../app/services/update/ClassNameUpdater'; +import { ClassNameBoundaryRegexBuilder } from '../domain/namespace/ClassNameBoundaryRegexBuilder'; import { ConfigurationLocator, Props } from '../domain/workspace/ConfigurationLocator'; import { FeatureFlagManager } from '../domain/workspace/FeatureFlagManager'; import { FileExtensionResolver } from '../domain/workspace/FileExtensionResolver'; @@ -39,6 +40,7 @@ function buildUpdater(additionalExtensions: string[]): ClassNameUpdater { new TextDocumentOpener(), workspacePathResolver, new FileEditApplier(fakeFeatureFlagManager()), + new ClassNameBoundaryRegexBuilder(), ); } @@ -116,4 +118,42 @@ suite('ClassNameUpdater', () => { const text = await waitForText(uri, t => t.includes('OutroTest.class')); assert.ok(text.includes('class OutroTest.class'), `expected today's known-bad output, got:\n${text}`); }); + + /** + * A class can share its name with a sibling namespace (e.g. a + * RenamedClass.php file next to a RenamedClass/ directory holding + * Foo/Type.php). When RenamedClass.php is renamed to + * RenamedClassTest.php, its own aliased imports from that sibling + * namespace must not be corrupted just because they start with the old name. + */ + test('does not corrupt its own aliased imports from a sub-namespace sharing its name', async () => { + const content = [ + ' t.includes('class RenamedClassTest')); + assert.ok( + text.includes('use App\\Controller\\RenamedClass\\Foo\\Type as FooType;'), + `aliased sub-namespace import should be left untouched, got:\n${text}`, + ); + assert.ok( + text.includes('use App\\Controller\\RenamedClass\\Bar\\Type as BarType;'), + `aliased sub-namespace import should be left untouched, got:\n${text}`, + ); + }); }); diff --git a/src/test/ClassTypedPropertyLocator.test.ts b/src/test/ClassTypedPropertyLocator.test.ts new file mode 100644 index 0000000..2da4d42 --- /dev/null +++ b/src/test/ClassTypedPropertyLocator.test.ts @@ -0,0 +1,125 @@ +import 'reflect-metadata'; + +import * as assert from 'assert'; + +import { ClassTypedPropertyLocator } from '../domain/property/ClassTypedPropertyLocator'; +import { ConstructorSpanFinder } from '../domain/property/ConstructorSpanFinder'; + +function locate(text: string, className: string) { + const locator = new ClassTypedPropertyLocator(new ConstructorSpanFinder()); + return locator.execute({ text, className }); +} + +suite('ClassTypedPropertyLocator', () => { + test('finds a promoted property', () => { + const text = [ + 'class UserController', + '{', + ' public function __construct(private Test $teste)', + ' {', + ' }', + '}', + ].join('\n'); + + const match = locate(text, 'Teste'); + assert.ok(match); + assert.strictEqual(match!.propertyName, 'teste'); + assert.strictEqual(match!.isPromoted, true); + assert.strictEqual(match!.hasSeparateDeclaration, false); + }); + + test('finds a promoted readonly property regardless of modifier order', () => { + const first = locate('function __construct(private readonly Teste $teste) {}', 'Teste'); + const second = locate('function __construct(readonly private Test $teste) {}', 'Teste'); + + assert.strictEqual(first!.propertyName, 'teste'); + assert.strictEqual(second!.propertyName, 'teste'); + }); + + test('finds a non-promoted property confirmed by a constructor assignment', () => { + const text = [ + 'class UserController', + '{', + ' private Test $teste;', + '', + ' public function __construct(Teste $teste)', + ' {', + ' $this->teste = $teste;', + ' }', + '}', + ].join('\n'); + + const match = locate(text, 'Teste'); + assert.ok(match); + assert.strictEqual(match!.propertyName, 'teste'); + assert.strictEqual(match!.isPromoted, false); + assert.strictEqual(match!.hasSeparateDeclaration, true); + }); + + test('finds an untyped property declared only via a @var docblock', () => { + const text = [ + 'class UserService', + '{', + ' /**', + ' * @var UserRepository', + ' */', + ' private $repository;', + '', + ' public function __construct(UserRepository $repository)', + ' {', + ' $this->repository = $repository;', + ' }', + '}', + ].join('\n'); + + const match = locate(text, 'UserRepository'); + assert.ok(match); + assert.strictEqual(match!.propertyName, 'repository'); + assert.strictEqual(match!.isPromoted, false); + assert.strictEqual(match!.hasSeparateDeclaration, true); + }); + + test('ignores a non-promoted parameter that is never stored on $this', () => { + const text = [ + 'class Validator', + '{', + ' public function __construct(Teste $teste)', + ' {', + ' $teste->validate();', + ' }', + '}', + ].join('\n'); + + assert.strictEqual(locate(text, 'Teste'), null); + }); + + test('matches a property with a mismatched name', () => { + const text = 'function __construct(private Test $service) {}'; + const match = locate(text, 'Teste'); + + assert.ok(match); + assert.strictEqual(match!.propertyName, 'service'); + }); + + test('matches a nullable type hint', () => { + const text = 'function __construct(private ?Teste $teste) {}'; + const match = locate(text, 'Teste'); + + assert.ok(match); + assert.strictEqual(match!.propertyName, 'teste'); + }); + + test('returns null when there is no constructor', () => { + assert.strictEqual(locate('class Teste {}', 'Teste'), null); + }); + + test('returns null when the class type does not appear in the constructor', () => { + const text = 'function __construct(private Other $other) {}'; + assert.strictEqual(locate(text, 'Teste'), null); + }); + + test('returns null when two parameters share the same type (ambiguous)', () => { + const text = 'function __construct(private Test $a, private Test $b) {}'; + assert.strictEqual(locate(text, 'Teste'), null); + }); +}); diff --git a/src/test/MultiFileReferenceUpdater.test.ts b/src/test/MultiFileReferenceUpdater.test.ts index b16b8af..a1c4be0 100644 --- a/src/test/MultiFileReferenceUpdater.test.ts +++ b/src/test/MultiFileReferenceUpdater.test.ts @@ -8,6 +8,7 @@ import * as vscode from 'vscode'; import { ImportRemover } from '../app/services/remove/ImportRemover'; import { MultiFileReferenceUpdater } from '../app/services/update/MultiFileReferenceUpdater'; +import { ClassNameBoundaryRegexBuilder } from '../domain/namespace/ClassNameBoundaryRegexBuilder'; import { UseStatementCreator } from '../domain/namespace/UseStatementCreator'; import { UseStatementInjector } from '../domain/namespace/UseStatementInjector'; import { UseStatementLocator } from '../domain/namespace/UseStatementLocator'; @@ -50,6 +51,7 @@ function buildUpdater(namespaceIndex: NamespaceIndex, editFilesInBackground = tr new UseStatementLocator(), new UseStatementInjector(fileEditApplier), fileEditApplier, + new ClassNameBoundaryRegexBuilder(), ); } @@ -239,4 +241,67 @@ suite('MultiFileReferenceUpdater', () => { ); assert.ok(!/\bprivate Order \$order\b/.test(text), `old bare class name should be gone, got:\n${text}`); }); + + /** + * A class can share its name with a sibling namespace (e.g. a + * RenamedClass.php file next to a RenamedClass/ directory holding + * Foo/FormType.php and Bar/FormType.php). Renaming the class + * to RenamedClassTest must not corrupt aliased imports that merely + * start with the old FQCN but actually point into that sibling namespace. + */ + test('renaming a class does not corrupt aliased imports from a sub-namespace sharing its name', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + + const oldUri = vscode.Uri.file(path.join(dir, 'RenamedClass.php')); + const newUri = vscode.Uri.file(path.join(dir, 'RenamedClassTest.php')); + + const consumerContent = [ + ' { /** @@ -79,3 +83,126 @@ suite('PHP_CLASS_DECLARATION_REGEX', () => { }); }); }); + +suite('NOT_PRECEDED_BY_NAMESPACE_CHAR / NOT_FOLLOWED_BY_NAMESPACE_CHAR', () => { + function escapeForRegex(text: string): string { + return text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + } + + function buildGuardedRegex(identifier: string): RegExp { + return new RegExp(`${NOT_PRECEDED_BY_NAMESPACE_CHAR}${identifier}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'); + } + + function buildGuardedNamespaceRegex(oldNamespace: string): RegExp { + return new RegExp(`${escapeForRegex(oldNamespace)}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'); + } + + /** + * Bug – a class "RenamedClass" can coexist with a sibling namespace of + * the same name (e.g. a RenamedClass.php file next to a + * RenamedClass/ directory). Renaming the class to "RenamedClassTeste" + * must not corrupt aliased imports from that sibling namespace, such as + * "use ...\RenamedClass\Foo\FormType as FooFormType;", + * because "RenamedClass" there is a namespace segment, not the class. + */ + suite('Guard against matching an identifier that is a namespace-path prefix of a longer one', () => { + test('replaces the identifier when it stands alone as a full FQCN', () => { + const regex = buildGuardedNamespaceRegex('App\\Controller\\Atendimento\\RenamedClass'); + const content = 'use App\\Controller\\Atendimento\\RenamedClass;'; + + const result = content.replace(regex, 'App\\Controller\\Atendimento\\RenamedClassTeste'); + + assert.strictEqual(result, 'use App\\Controller\\Atendimento\\RenamedClassTeste;'); + }); + + test('does not touch aliased imports from a deeper sub-namespace sharing the same prefix', () => { + const regex = buildGuardedRegex('RenamedClass'); + const content = [ + 'use App\\Controller\\Atendimento\\RenamedClass\\Foo\\FormType as FooFormType;', + 'use App\\Controller\\Atendimento\\RenamedClass\\Bar\\FormType as BarFormType;', + ].join('\n'); + + const result = content.replace(regex, 'RenamedClassTeste'); + + assert.strictEqual(result, content, `sub-namespace imports should be left untouched, got:\n${result}`); + }); + + test('still replaces the bare identifier when it is not part of a namespace path', () => { + const regex = buildGuardedRegex('RenamedClass'); + const content = 'private RenamedClass $RenamedClass;\n\nnew RenamedClass();'; + + const result = content.replace(regex, 'RenamedClassTeste'); + + assert.strictEqual( + result, + 'private RenamedClassTeste $RenamedClass;\n\nnew RenamedClassTeste();', + ); + }); + + test('does not match a suffix identifier that merely starts with the same characters', () => { + const regex = buildGuardedRegex('RenamedClass'); + const content = 'use App\\Domain\\RenamedClassAbstract;'; + + const result = content.replace(regex, 'RenamedClassTeste'); + + assert.strictEqual(result, content); + }); + + test('without the guard, the old regex would corrupt the sub-namespace imports (regression check)', () => { + const unguardedRegex = new RegExp('RenamedClass', 'g'); + const content = 'use App\\Controller\\Atendimento\\RenamedClass\\Foo\\FormType as FooFormType;'; + + const result = content.replace(unguardedRegex, 'RenamedClassTeste'); + + assert.strictEqual( + result, + 'use App\\Controller\\Atendimento\\RenamedClassTeste\\Foo\\FormType as FooFormType;', + ); + }); + }); + + /** + * Bug – renaming "DetalhePagamentoDTO" to "DetalhePagamentoDTOAbstract" was + * corrupting an unrelated "use ...DetalhePagamentoDTOAbstract;" statement + * already present in the same file, turning it into + * "...DetalhePagamentoDTOAbstractAbstract" because the namespace replace + * regex matched the old FQCN as a mere prefix of the longer one. + */ + suite('Guard against matching a FQCN that is a prefix of a longer identifier', () => { + test('replaces the old namespace when it is not followed by extra identifier characters', () => { + const regex = buildGuardedNamespaceRegex('SharedBundle\\DetalhePagamentoDTO'); + const content = 'use SharedBundle\\DetalhePagamentoDTO;'; + + const result = content.replace(regex, 'SharedBundle\\DetalhePagamentoDTOAbstract'); + + assert.strictEqual(result, 'use SharedBundle\\DetalhePagamentoDTOAbstract;'); + }); + + test('does not touch an unrelated FQCN that has the old namespace as a prefix', () => { + const regex = buildGuardedNamespaceRegex('SharedBundle\\DetalhePagamentoDTO'); + const content = [ + 'use SharedBundle\\DetalhePagamentoDTO;', + 'use SharedBundle\\DetalhePagamentoDTOAbstract;', + ].join('\n'); + + const result = content.replace(regex, 'SharedBundle\\DetalhePagamentoDTOAbstract'); + + assert.strictEqual( + result, + [ + 'use SharedBundle\\DetalhePagamentoDTOAbstract;', + 'use SharedBundle\\DetalhePagamentoDTOAbstract;', + ].join('\n'), + ); + }); + + test('without the guard, the old regex would double the suffix (regression check)', () => { + const unguardedRegex = new RegExp('SharedBundle\\\\DetalhePagamentoDTO', 'g'); + const content = 'use SharedBundle\\DetalhePagamentoDTOAbstract;'; + + const result = content.replace(unguardedRegex, 'SharedBundle\\DetalhePagamentoDTOAbstract'); + + assert.strictEqual(result, 'use SharedBundle\\DetalhePagamentoDTOAbstractAbstract;'); + }); + }); +}); diff --git a/src/test/PropertyRenameOperation.test.ts b/src/test/PropertyRenameOperation.test.ts new file mode 100644 index 0000000..4507bdb --- /dev/null +++ b/src/test/PropertyRenameOperation.test.ts @@ -0,0 +1,193 @@ +import 'reflect-metadata'; + +import * as assert from 'assert'; +import { promises as fs } from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import * as vscode from 'vscode'; + +import { PropertyRenameOperation } from '../app/operations/PropertyRenameOperation'; +import { ClassTypedPropertyLocator } from '../domain/property/ClassTypedPropertyLocator'; +import { ConstructorSpanFinder } from '../domain/property/ConstructorSpanFinder'; +import { PropertyNameResolver } from '../domain/property/PropertyNameResolver'; +import { ConfigurationLocator, Props } from '../domain/workspace/ConfigurationLocator'; +import { FeatureFlagManager } from '../domain/workspace/FeatureFlagManager'; +import { FileExtensionResolver } from '../domain/workspace/FileExtensionResolver'; +import { WorkspacePathResolver } from '../domain/workspace/WorkspacePathResolver'; +import { ComposerAutoloadManager } from '../infra/autoload/ComposerAutoloadManager'; +import { FileEditApplier } from '../infra/vscode/FileEditApplier'; +import { TextDocumentOpener } from '../infra/vscode/TextDocumentOpener'; + +function fakePassthroughConfigurationLocator(): ConfigurationLocator { + return { + get: ({ defaultValue }: Props): T => defaultValue as T, + } as ConfigurationLocator; +} + +function fakeFeatureFlagManager(editFilesInBackground = true): FeatureFlagManager { + return { + isActive: ({ defaultValue = true }) => defaultValue && editFilesInBackground, + } as FeatureFlagManager; +} + +function buildOperation({ + editFilesInBackground = true, +} = {}): PropertyRenameOperation { + const workspacePathResolver = new WorkspacePathResolver( + new ComposerAutoloadManager(), + new FileExtensionResolver(fakePassthroughConfigurationLocator()), + ); + const fileEditApplier = new FileEditApplier(fakeFeatureFlagManager(editFilesInBackground)); + const constructorSpanFinder = new ConstructorSpanFinder(); + + return new PropertyRenameOperation( + workspacePathResolver, + new TextDocumentOpener(), + fileEditApplier, + new ClassTypedPropertyLocator(constructorSpanFinder), + new PropertyNameResolver(), + constructorSpanFinder, + ); +} + +async function writeTempPhpFile(dir: string, fileName: string, content: string): Promise { + const filePath = path.join(dir, fileName); + await fs.writeFile(filePath, content, 'utf8'); + return vscode.Uri.file(filePath); +} + +suite('PropertyRenameOperation', () => { + test('renames a promoted property that matches the old class-name convention', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = 'teste->run();\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); + + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: false }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private NewTest $newTest'), `expected the promoted property to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->novo->run();'), `expected $this-> usages to be renamed, got:\n${text}`); + assert.ok(!text.includes('teste'), `expected no leftover old property name, got:\n${text}`); + }); + + test('renames a non-promoted property confirmed by its constructor assignment', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = 'teste = $teste;\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); + + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: false }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private NewTest $newTest;'), `expected the declared property to be renamed, got:\n${text}`); + assert.ok(text.includes('__construct(Novo $novo)'), `expected the constructor parameter to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->novo = $novo;'), `expected the assignment to be renamed, got:\n${text}`); + }); + + test('renames an untyped property declared only via a @var docblock', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'UserRepository.php')); + const newUri = vscode.Uri.file(path.join(dir, 'ClienteRepository.php')); + + const consumerContent = 'repository = $repository;\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserService.php', consumerContent); + + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: true }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private $clienteRepository;'), `expected the untyped declaration to be renamed, got:\n${text}`); + assert.ok(text.includes('__construct(ClienteRepository $clienteRepository)'), `expected the constructor parameter to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->clienteRepository = $clienteRepository;'), `expected the assignment to be renamed, got:\n${text}`); + assert.ok(!text.includes('$repository'), `expected no leftover old property name, got:\n${text}`); + }); + + test('leaves a mismatched property name untouched when renameMismatchedNames is false', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = ' { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = 'service->run();\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); + + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: true }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private NewTest $newTest'), `expected the mismatched property to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->novo->run();'), `expected $this-> usages to be renamed, got:\n${text}`); + }); + + test('skips a file when two properties share the renamed class type (ambiguous)', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = ' { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const sameUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + + const consumerContent = ' { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const unrelatedContent = ' rawValue, + } as unknown as ConfigurationLocator; + + return new PropertyRenameSettingsResolver(configurationLocator); +} + +suite('PropertyRenameSettingsResolver', () => { + test('resolves false to disabled', () => { + assert.deepStrictEqual( + buildResolver(false).resolve(), + { enabled: false, renameMismatchedNames: false }, + ); + }); + + test('resolves undefined (unset) to disabled', () => { + assert.deepStrictEqual( + buildResolver(undefined).resolve(), + { enabled: false, renameMismatchedNames: false }, + ); + }); + + test('resolves true to enabled, without the mismatch behavior', () => { + assert.deepStrictEqual( + buildResolver(true).resolve(), + { enabled: true, renameMismatchedNames: false }, + ); + }); + + test('resolves an empty object to enabled, without the mismatch behavior', () => { + assert.deepStrictEqual( + buildResolver({}).resolve(), + { enabled: true, renameMismatchedNames: false }, + ); + }); + + test('resolves { renameMismatchedNames: true } to enabled with the mismatch behavior on', () => { + assert.deepStrictEqual( + buildResolver({ renameMismatchedNames: true }).resolve(), + { enabled: true, renameMismatchedNames: true }, + ); + }); + + test('resolves { renameMismatchedNames: false } to enabled with the mismatch behavior off', () => { + assert.deepStrictEqual( + buildResolver({ renameMismatchedNames: false }).resolve(), + { enabled: true, renameMismatchedNames: false }, + ); + }); +}); diff --git a/src/test/UnusedImportDetector.test.ts b/src/test/UnusedImportDetector.test.ts index 5566923..2a2609e 100644 --- a/src/test/UnusedImportDetector.test.ts +++ b/src/test/UnusedImportDetector.test.ts @@ -2,13 +2,14 @@ import 'reflect-metadata'; import * as assert from 'assert'; +import { ClassNameBoundaryRegexBuilder } from '../domain/namespace/ClassNameBoundaryRegexBuilder'; import { UnusedImportDetector } from '../domain/namespace/UnusedImportDetector'; suite('UnusedImportDetector', () => { let detector: UnusedImportDetector; setup(() => { - detector = new UnusedImportDetector(); + detector = new UnusedImportDetector(new ClassNameBoundaryRegexBuilder()); }); /** @@ -110,5 +111,25 @@ suite('UnusedImportDetector', () => { const result = detector.execute({ contentDocument: content, classes: ['UserService'] }); assert.strictEqual(result.length, 1); }); + + /** + * A class can share its name with a sibling namespace (e.g. a + * RenamedClass.php file next to a RenamedClass/ directory). An + * aliased import from that sibling namespace, such as + * "use ...\RenamedClass\Foo\FormType as FooFormType;", + * must not make "RenamedClass" look used — it's a namespace segment + * there, not a reference to the class. + */ + test('does not treat a class name as used merely because it prefixes a sub-namespace path', () => { + const content = [ + 'namespace App\\Controller;', + 'use App\\Controller\\RenamedClass\\Foo\\FormType as FooFormType;', + 'use App\\Controller\\RenamedClass\\Bar\\FormType as BarFormType;', + 'class Foo { function bar(FooFormType $f) {} }', + ].join('\n'); + + const result = detector.execute({ contentDocument: content, classes: ['RenamedClass'] }); + assert.deepStrictEqual(result, []); + }); }); });