From 2cb181a7d1a2477864bab4bb77592892e6a9d8ed Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 02:34:50 +0700 Subject: [PATCH 01/19] feat: Add YagniPreset to detect and remove unused interfaces, abstract classes, and traits on scanned paths --- docs/available-rules.md | 5 +- docs/presets.md | 3 + src/Analyser/Analyser.php | 97 +++++++- src/Analyser/AnonymousClassNode.php | 14 +- src/Analyser/ClassCollector.php | 11 +- src/Analyser/ClassNode.php | 23 ++ src/Cache/AnalysisResultCache.php | 28 ++- src/Cli/InitCommand.php | 4 +- src/Cli/Usage.php | 2 +- src/Preset/Preset.php | 13 ++ src/Preset/Presets/YagniPreset.php | 59 +++++ .../AbstractPhpParserFixableRule.php | 11 + .../ClassLike/RemoveClassLikeVisitor.php | 36 +++ .../PhpParser/PhpParserFixerProcessor.php | 44 +++- ...ImplementedInterfaceAwareRuleInterface.php | 20 ++ .../Class_/MustBeImplementedInterfaceRule.php | 68 ++++++ .../MustBeOverriddenAbstractClassRule.php | 68 ++++++ src/Rule/Rules/Class_/MustBeUsedTraitRule.php | 68 ++++++ src/Rule/UsedTraitAwareRuleInterface.php | 18 ++ tests/Analyser/AnalyserTest.php | 162 +++++++++++++ tests/Analyser/ClassNodeTest.php | 47 ++++ tests/Cache/AnalysisResultCacheTest.php | 14 +- tests/Cli/InitCommandTest.php | 8 +- ...ructArmedApplicationCommandRoutingTest.php | 2 +- tests/Cli/StructArmedApplicationTest.php | 8 +- tests/Preset/PresetTest.php | 40 ++++ .../MustBeImplementedInterfaceRuleTest.php | 213 ++++++++++++++++++ .../MustBeOverriddenAbstractClassRuleTest.php | 190 ++++++++++++++++ tests/Rule/Class_/MustBeUsedTraitRuleTest.php | 169 ++++++++++++++ .../ClassLike/RemoveClassLikeVisitorTest.php | 80 +++++++ 30 files changed, 1492 insertions(+), 33 deletions(-) create mode 100644 src/Preset/Presets/YagniPreset.php create mode 100644 src/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitor.php create mode 100644 src/Rule/ImplementedInterfaceAwareRuleInterface.php create mode 100644 src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php create mode 100644 src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php create mode 100644 src/Rule/Rules/Class_/MustBeUsedTraitRule.php create mode 100644 src/Rule/UsedTraitAwareRuleInterface.php create mode 100644 tests/Rule/Class_/MustBeImplementedInterfaceRuleTest.php create mode 100644 tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php create mode 100644 tests/Rule/Class_/MustBeUsedTraitRuleTest.php create mode 100644 tests/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitorTest.php diff --git a/docs/available-rules.md b/docs/available-rules.md index 88bddfc4..284e3f0f 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -81,7 +81,10 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | +| `MustBeImplementedInterfaceRule` | `new MustBeImplementedInterfaceRule(layer: 'Source')` | Interfaces are implemented by a scanned class (directly or through inheritance) or extended by another scanned interface. Supports `--fix` by removing the unused interface (and deleting its file when only boilerplate remains). | | `MustBeInterfaceRule` | `new MustBeInterfaceRule(layer: 'Contract', classNamePattern: '/Interface$/')` | Matching declarations in a layer are interfaces. | +| `MustBeOverriddenAbstractClassRule` | `new MustBeOverriddenAbstractClassRule(layer: 'Source')` | Abstract classes are extended by a scanned class. Supports `--fix` by removing the unused abstract class (and deleting its file when only boilerplate remains). | +| `MustBeUsedTraitRule` | `new MustBeUsedTraitRule(layer: 'Source')` | Traits are used by a scanned class, trait, or enum. Supports `--fix` by removing the unused trait (and deleting its file when only boilerplate remains). | | `MustDeclareConstantVisibilityRule` | `new MustDeclareConstantVisibilityRule(layer: 'Source')` | Class constants declare `public`, `protected`, or `private`. Supports `--fix`. | | `MustDeclareMethodVisibilityRule` | `new MustDeclareMethodVisibilityRule(layer: 'Source')` | Methods declare `public`, `protected`, or `private`. Supports `--fix`. | | `MustDeclarePropertyVisibilityRule` | `new MustDeclarePropertyVisibilityRule(layer: 'Source')` | Properties declare `public`, `protected`, or `private`. Supports `--fix`. | @@ -91,7 +94,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. `classNamePattern` and `excludePattern` are regular expressions matched against the fully-qualified class name. -`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `MustBeFinalRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` class modifier, and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. +`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `MustBeFinalRule`, `MustBeImplementedInterfaceRule`, `MustBeOverriddenAbstractClassRule`, `MustBeUsedTraitRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` class modifier, remove unused interfaces, abstract classes, and traits (deleting their file when only `declare`/`namespace`/`use` boilerplate remains), and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. ## Layer Rules diff --git a/docs/presets.md b/docs/presets.md index 5b638e5c..7104ad2f 100644 --- a/docs/presets.md +++ b/docs/presets.md @@ -25,6 +25,7 @@ StructArmed ships with presets for common PHP standards and architecture styles. | `Preset::PSR4()` | Verifies configured source paths exist in composer.json `autoload` or `autoload-dev` PSR-4 mappings | | `Preset::DDD()` | Layer isolation, entity/VO/repository/event/service conventions | | `Preset::MVC()` | Layer isolation, thin controllers, model/view/service rules | +| `Preset::YAGNI()` | Speculative-abstraction cleanup: interfaces must be implemented by a class or extended by another interface, abstract classes must be extended, traits must be used within the scanned paths. All three rules support `--fix` by removing the unused declaration | ## Initialize Presets @@ -35,6 +36,7 @@ vendor/bin/structarmed init --preset=psr12 vendor/bin/structarmed init --preset=psr15 vendor/bin/structarmed init --preset=mvc vendor/bin/structarmed init --preset=ddd +vendor/bin/structarmed init --preset=yagni vendor/bin/structarmed init --preset=all ``` @@ -49,6 +51,7 @@ return Architecture::define() Preset::PSR15(), Preset::MVC(), Preset::DDD(), + Preset::YAGNI(), ); ``` diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 33747a25..b3c2c4ef 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -17,6 +17,7 @@ use Boundwize\StructArmed\Rule\ExtendedClassAwareRuleInterface; use Boundwize\StructArmed\Rule\FileAnalysisRuleInterface; use Boundwize\StructArmed\Rule\FixableInterface; +use Boundwize\StructArmed\Rule\ImplementedInterfaceAwareRuleInterface; use Boundwize\StructArmed\Rule\LayerAwareRuleInterface; use Boundwize\StructArmed\Rule\MultipleProjectRuleViolationInterface; use Boundwize\StructArmed\Rule\MultipleRuleViolationInterface; @@ -24,6 +25,7 @@ use Boundwize\StructArmed\Rule\RuleInterface; use Boundwize\StructArmed\Rule\RuleViolation; use Boundwize\StructArmed\Rule\RuleViolationCollection; +use Boundwize\StructArmed\Rule\UsedTraitAwareRuleInterface; use Boundwize\StructArmed\Util\Path; use function array_fill_keys; @@ -78,11 +80,13 @@ public function analyse( $ruleSkipPaths = $architecture->getRuleSkipPaths(); $skippedRuleKeys = $this->skippedRuleKeyMap($architecture->getSkippedRuleKeys()); - $projectRuleViolations = []; - $fileAnalysisRules = []; - $classRules = []; - $layerAwareRules = []; - $hasExtendedClassAwareRule = false; + $projectRuleViolations = []; + $fileAnalysisRules = []; + $classRules = []; + $layerAwareRules = []; + $hasExtendedClassAwareRule = false; + $hasImplementedInterfaceAwareRule = false; + $hasUsedTraitAwareRule = false; foreach ($rules as $key => $rule) { if (array_key_exists($key, $skippedRuleKeys)) { @@ -101,6 +105,14 @@ public function analyse( $hasExtendedClassAwareRule = true; } + if ($rule instanceof ImplementedInterfaceAwareRuleInterface) { + $hasImplementedInterfaceAwareRule = true; + } + + if ($rule instanceof UsedTraitAwareRuleInterface) { + $hasUsedTraitAwareRule = true; + } + if (! $rule instanceof ProjectRuleInterface) { continue; } @@ -152,6 +164,14 @@ public function analyse( $this->markExtendedClasses($classNodes, $extractionResult); } + if ($hasImplementedInterfaceAwareRule) { + $this->markImplementedInterfaces($classNodes, $extractionResult); + } + + if ($hasUsedTraitAwareRule) { + $this->markUsedTraits($classNodes, $extractionResult); + } + if ($withFileAnalysis) { $fileAnalysisProvider = new FileAnalysisProvider( analyses: $extractionResult->fileAnalyses, @@ -688,6 +708,73 @@ private function markExtendedClasses(array $classNodes, ExtractionResult $extrac } } + /** + * Flag every interface that a scanned class implements (directly or through + * inheritance) or another scanned interface extends, using the recursive + * parent chain resolved by {@see withRecursiveParents()}. + * + * @param list $classNodes + */ + private function markImplementedInterfaces(array $classNodes, ExtractionResult $extractionResult): void + { + $implemented = []; + + foreach ($classNodes as $classNode) { + foreach ($classNode->parentInterfaces as $parentInterface) { + $implemented[strtolower($parentInterface)] = true; + } + } + + // Anonymous classes (`new class implements Foo {}`) have no ClassNode + // of their own, so the interfaces they implement are tracked separately. + foreach ($extractionResult->anonymousClassNodes as $anonymousClassNode) { + foreach ($anonymousClassNode->implements as $interface) { + $implemented[strtolower($interface)] = true; + } + } + + foreach ($classNodes as $classNode) { + // Only interfaces appear in parentInterfaces; classes, traits, and + // enums never do, so they are left with the default (not implemented). + if (isset($implemented[strtolower($classNode->className)])) { + $classNode->setImplemented(true); + } + } + } + + /** + * Flag every trait that another scanned class-like (class, trait, or enum) + * uses. Trait usage is a direct declaration, so no recursive chain is + * needed: a trait used only by another unused trait stays flagged as used + * until that trait is removed, at which point the next run reports it. + * + * @param list $classNodes + */ + private function markUsedTraits(array $classNodes, ExtractionResult $extractionResult): void + { + $used = []; + + foreach ($classNodes as $classNode) { + foreach ($classNode->traits as $trait) { + $used[strtolower($trait)] = true; + } + } + + // Anonymous classes (`new class { use Foo; }`) have no ClassNode of + // their own, so the traits they use are tracked separately. + foreach ($extractionResult->anonymousClassNodes as $anonymousClassNode) { + foreach ($anonymousClassNode->traits as $trait) { + $used[strtolower($trait)] = true; + } + } + + foreach ($classNodes as $classNode) { + if (isset($used[strtolower($classNode->className)])) { + $classNode->setUsed(true); + } + } + } + /** * @param list $classNodes * @return list diff --git a/src/Analyser/AnonymousClassNode.php b/src/Analyser/AnonymousClassNode.php index dcdac5af..c1f85c57 100644 --- a/src/Analyser/AnonymousClassNode.php +++ b/src/Analyser/AnonymousClassNode.php @@ -7,20 +7,24 @@ /** * An anonymous class declaration (`new class ... {}`). Anonymous classes never * become ClassNodes — they cannot be referenced by name and no rule targets - * them directly — but the class they extend is still extended within the - * scanned paths, which extended-class-aware rules must take into account. + * them directly — but the class they extend, the interfaces they implement, + * and the traits they use are still used within the scanned paths, which + * usage-aware rules must take into account. * * The usage example is on MustBeFinalRule, which must skip if target class is extended by an anonymous class. - * - * Note: Other properties like anonymous class's traits, implements, etc may come - * later if needed for future needed rules. */ final readonly class AnonymousClassNode { + /** + * @param string[] $implements Interface names this anonymous class implements + * @param string[] $traits Trait names this anonymous class uses + */ public function __construct( public string $file, public int $line, public ?string $extends, + public array $implements = [], + public array $traits = [], ) { } } diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index 8ccd532a..2ca9cc0e 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -209,12 +209,15 @@ public function leaveNode(Node $node): null if (! $node->name instanceof Identifier) { // Anonymous classes never become ClassNodes, but the class they - // extend is still extended within the scanned paths. + // extend, the interfaces they implement, and the traits they use + // are still used within the scanned paths. if ($node instanceof Class_) { $this->anonymousClassNodes[] = new AnonymousClassNode( - file: $this->currentFile, - line: $node->getStartLine(), - extends: $node->extends instanceof Name ? $node->extends->toString() : null, + file: $this->currentFile, + line: $node->getStartLine(), + extends: $node->extends instanceof Name ? $node->extends->toString() : null, + implements: $this->collectImplements($node), + traits: $this->collectTraits($node), ); } diff --git a/src/Analyser/ClassNode.php b/src/Analyser/ClassNode.php index f4b88c74..45b8bb6a 100644 --- a/src/Analyser/ClassNode.php +++ b/src/Analyser/ClassNode.php @@ -60,6 +60,8 @@ public function __construct( public array $parentClasses = [], public array $parentInterfaces = [], public bool $isExtended = false, + public bool $isImplemented = false, + public bool $isUsed = false, ) { $this->layers = $layers ?: array_filter([$this->layer]); } @@ -83,6 +85,27 @@ public function setExtended(bool $isExtended): void $this->isExtended = $isExtended; } + /** + * Whether another scanned class implements this interface (directly or + * through inheritance) or another scanned interface extends it. Computed by + * the analyser for rules implementing ImplementedInterfaceAwareRuleInterface; + * false otherwise. + */ + public function setImplemented(bool $isImplemented): void + { + $this->isImplemented = $isImplemented; + } + + /** + * Whether another scanned class-like uses this trait. Computed by the + * analyser for rules implementing UsedTraitAwareRuleInterface; false + * otherwise. + */ + public function setUsed(bool $isUsed): void + { + $this->isUsed = $isUsed; + } + public function shortName(): string { $parts = explode('\\', $this->className); diff --git a/src/Cache/AnalysisResultCache.php b/src/Cache/AnalysisResultCache.php index d6eb2016..822be297 100644 --- a/src/Cache/AnalysisResultCache.php +++ b/src/Cache/AnalysisResultCache.php @@ -388,9 +388,11 @@ className: $className, private function anonymousClassNodeToArray(AnonymousClassNode $anonymousClassNode): array { return [ - 'file' => $anonymousClassNode->file, - 'line' => $anonymousClassNode->line, - 'extends' => $anonymousClassNode->extends, + 'file' => $anonymousClassNode->file, + 'line' => $anonymousClassNode->line, + 'extends' => $anonymousClassNode->extends, + 'implements' => $anonymousClassNode->implements, + 'traits' => $anonymousClassNode->traits, ]; } @@ -413,18 +415,26 @@ private function anonymousClassNodesFromPayload(array $payload): ?array return null; } - $file = $rawNode['file'] ?? null; - $line = $rawNode['line'] ?? null; - $extends = $rawNode['extends'] ?? null; + $file = $rawNode['file'] ?? null; + $line = $rawNode['line'] ?? null; + $extends = $rawNode['extends'] ?? null; + $implements = $rawNode['implements'] ?? []; + $traits = $rawNode['traits'] ?? []; if (! is_string($file) || ! is_int($line) || ($extends !== null && ! is_string($extends))) { return null; } + if (! $this->isStringArray($implements) || ! $this->isStringArray($traits)) { + return null; + } + $anonymousClassNodes[] = new AnonymousClassNode( - file: $file, - line: $line, - extends: $extends, + file: $file, + line: $line, + extends: $extends, + implements: $implements, + traits: $traits, ); } diff --git a/src/Cli/InitCommand.php b/src/Cli/InitCommand.php index 446ac9ef..be1ae3c7 100644 --- a/src/Cli/InitCommand.php +++ b/src/Cli/InitCommand.php @@ -87,13 +87,15 @@ private function presetConfig(string $preset): ?string 'psr12' => ' ->withPreset(Preset::PSR12());', 'psr15' => ' ->withPreset(Preset::PSR15());', 'psr4' => ' ->withPreset(Preset::PSR4());', + 'yagni' => ' ->withPreset(Preset::YAGNI());', 'all' => " ->withPresets(\n" . " Preset::PSR1(),\n" . " Preset::PSR12(),\n" . " Preset::PSR15(),\n" . " Preset::PSR4(),\n" . " Preset::DDD(),\n" - . " Preset::MVC()\n" + . " Preset::MVC(),\n" + . " Preset::YAGNI()\n" . " );", default => null, }; diff --git a/src/Cli/Usage.php b/src/Cli/Usage.php index 29890744..1752c038 100644 --- a/src/Cli/Usage.php +++ b/src/Cli/Usage.php @@ -11,7 +11,7 @@ public static function render(): string return <<<'TXT' Usage: structarmed --version - structarmed init [--preset=ddd|mvc|psr1|psr12|psr15|psr4|all] + structarmed init [--preset=ddd|mvc|psr1|psr12|psr15|psr4|yagni|all] structarmed analyse|analyze [path ...] [--config=path/to/structarmed.php] [--report=console|json] [--no-progress] [--clear-cache] [--disable-parallel] [--fix] [--generate-baseline=structarmed-baseline.php] diff --git a/src/Preset/Preset.php b/src/Preset/Preset.php index a2c28e0c..5124f601 100644 --- a/src/Preset/Preset.php +++ b/src/Preset/Preset.php @@ -10,6 +10,7 @@ use Boundwize\StructArmed\Preset\Presets\Psr15Preset; use Boundwize\StructArmed\Preset\Presets\Psr1Preset; use Boundwize\StructArmed\Preset\Presets\Psr4Preset; +use Boundwize\StructArmed\Preset\Presets\YagniPreset; /** * Factory for built-in presets. @@ -21,6 +22,7 @@ * ->withPreset(Preset::PSR4()) * ->withPreset(Preset::PSR12()) * ->withPreset(Preset::PSR15()) + * ->withPreset(Preset::YAGNI()) * ->withPresets(Preset::DDD(), Preset::MVC()) */ final class Preset @@ -85,6 +87,17 @@ public static function DDD( ); } + /** + * @param list|null $sourcePaths + */ + public static function YAGNI( + ?array $sourcePaths = null, + ): YagniPreset { + return new YagniPreset( + sourcePaths: $sourcePaths, + ); + } + public static function MVC( int $controllerMaxComplexity = 5, int $controllerMaxMethodLength = 20, diff --git a/src/Preset/Presets/YagniPreset.php b/src/Preset/Presets/YagniPreset.php new file mode 100644 index 00000000..0aea4ede --- /dev/null +++ b/src/Preset/Presets/YagniPreset.php @@ -0,0 +1,59 @@ +|null $sourcePaths + */ + public function __construct( + private ?array $sourcePaths = null, + ) { + } + + public function apply(Architecture $architecture): void + { + $layerName = $this->resolveLayerName($architecture); + $architecture->layer($layerName, $this->sourcePaths ?? []); + + $architecture->rule( + self::INTERFACE_MUST_BE_IMPLEMENTED, + new MustBeImplementedInterfaceRule($layerName) + ); + $architecture->rule( + self::ABSTRACT_CLASS_MUST_BE_OVERRIDDEN, + new MustBeOverriddenAbstractClassRule($layerName) + ); + $architecture->rule( + self::TRAIT_MUST_BE_USED, + new MustBeUsedTraitRule($layerName) + ); + } +} diff --git a/src/Rule/Fixer/PhpParser/AbstractPhpParserFixableRule.php b/src/Rule/Fixer/PhpParser/AbstractPhpParserFixableRule.php index 692e6738..a6edc60c 100644 --- a/src/Rule/Fixer/PhpParser/AbstractPhpParserFixableRule.php +++ b/src/Rule/Fixer/PhpParser/AbstractPhpParserFixableRule.php @@ -17,11 +17,22 @@ final public function fix(RuleViolation $ruleViolation): bool return $this->fixerProcessor()->process( $ruleViolation->file, $nodeVisitor, + $this->shouldRemoveFileWhenEmpty(), ); } abstract protected function createFixerVisitor(RuleViolation $ruleViolation): NodeVisitor; + /** + * Whether the fixed file should be deleted when the fix leaves no code + * behind — only declare/namespace/use boilerplate. Rules whose fix removes + * whole declarations opt in by returning true. + */ + protected function shouldRemoveFileWhenEmpty(): bool + { + return false; + } + private function fixerProcessor(): PhpParserFixerProcessor { static $processor; diff --git a/src/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitor.php b/src/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitor.php new file mode 100644 index 00000000..4f97a9db --- /dev/null +++ b/src/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitor.php @@ -0,0 +1,36 @@ +namespacedName)) { + return null; + } + + if ($node->namespacedName->toString() !== $this->className) { + return null; + } + + return NodeVisitor::REMOVE_NODE; + } +} diff --git a/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php b/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php index f8b634f4..4e0c77f5 100644 --- a/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php +++ b/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php @@ -5,6 +5,12 @@ namespace Boundwize\StructArmed\Rule\Fixer\PhpParser; use PhpParser\Error; +use PhpParser\Node; +use PhpParser\Node\Stmt\Declare_; +use PhpParser\Node\Stmt\GroupUse; +use PhpParser\Node\Stmt\Namespace_; +use PhpParser\Node\Stmt\Nop; +use PhpParser\Node\Stmt\Use_; use PhpParser\NodeTraverser; use PhpParser\NodeVisitor; use PhpParser\NodeVisitor\CloningVisitor; @@ -15,10 +21,11 @@ use function file_get_contents; use function file_put_contents; use function is_file; +use function unlink; final readonly class PhpParserFixerProcessor { - public function process(string $file, NodeVisitor $nodeVisitor): bool + public function process(string $file, NodeVisitor $nodeVisitor, bool $removeFileWhenEmpty = false): bool { if (! is_file($file)) { return false; @@ -43,8 +50,43 @@ public function process(string $file, NodeVisitor $nodeVisitor): bool ->traverse((new NodeTraverser(new CloningVisitor())) ->traverse($originalStatements)); + // A fix that removes the last declaration leaves only boilerplate + // (declare/namespace/use); the whole file is dead weight at that point. + if ($removeFileWhenEmpty && $this->hasOnlyDeclarations($statements)) { + return unlink($file); + } + $fixedCode = (new Standard())->printFormatPreserving($statements, $originalStatements, $parser->getTokens()); return $fixedCode !== $code && file_put_contents($file, $fixedCode) !== false; } + + /** + * @param Node[] $statements + */ + private function hasOnlyDeclarations(array $statements): bool + { + foreach ($statements as $statement) { + if ( + $statement instanceof Declare_ + || $statement instanceof Use_ + || $statement instanceof GroupUse + || $statement instanceof Nop + ) { + continue; + } + + if ($statement instanceof Namespace_) { + if (! $this->hasOnlyDeclarations($statement->stmts)) { + return false; + } + + continue; + } + + return false; + } + + return true; + } } diff --git a/src/Rule/ImplementedInterfaceAwareRuleInterface.php b/src/Rule/ImplementedInterfaceAwareRuleInterface.php new file mode 100644 index 00000000..59739bfd --- /dev/null +++ b/src/Rule/ImplementedInterfaceAwareRuleInterface.php @@ -0,0 +1,20 @@ +isImplemented. + * + * Trade-off: only usage within the scanned paths is known. An interface + * implemented solely by a consumer outside the scan is reported as if not + * implemented. + */ +interface ImplementedInterfaceAwareRuleInterface extends RuleInterface +{ +} diff --git a/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php b/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php new file mode 100644 index 00000000..0ef41b3c --- /dev/null +++ b/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php @@ -0,0 +1,68 @@ +isInterface) { + return false; + } + + if (! $classNode->isInLayer($this->layer)) { + return false; + } + + if ($this->classNamePattern !== null) { + return $classNode->nameMatches($this->classNamePattern, isFullName: true); + } + + return true; + } + + public function evaluate(ClassNode $classNode): ?RuleViolation + { + if ($classNode->isImplemented) { + return null; + } + + return new RuleViolation( + message: sprintf( + 'Interface [%s] must be implemented by a class or extended by another interface', + $classNode->className + ), + file: $classNode->file, + line: $classNode->line, + className: $classNode->className, + layer: $classNode->layer, + ); + } + + protected function createFixerVisitor(RuleViolation $ruleViolation): RemoveClassLikeVisitor + { + return new RemoveClassLikeVisitor($ruleViolation->className); + } + + protected function shouldRemoveFileWhenEmpty(): bool + { + return true; + } +} diff --git a/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php b/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php new file mode 100644 index 00000000..4d695d11 --- /dev/null +++ b/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php @@ -0,0 +1,68 @@ +isClass() || ! $classNode->isAbstract) { + return false; + } + + if (! $classNode->isInLayer($this->layer)) { + return false; + } + + if ($this->classNamePattern !== null) { + return $classNode->nameMatches($this->classNamePattern, isFullName: true); + } + + return true; + } + + public function evaluate(ClassNode $classNode): ?RuleViolation + { + if ($classNode->isExtended) { + return null; + } + + return new RuleViolation( + message: sprintf( + 'Abstract class [%s] must be extended by a class', + $classNode->className + ), + file: $classNode->file, + line: $classNode->line, + className: $classNode->className, + layer: $classNode->layer, + ); + } + + protected function createFixerVisitor(RuleViolation $ruleViolation): RemoveClassLikeVisitor + { + return new RemoveClassLikeVisitor($ruleViolation->className); + } + + protected function shouldRemoveFileWhenEmpty(): bool + { + return true; + } +} diff --git a/src/Rule/Rules/Class_/MustBeUsedTraitRule.php b/src/Rule/Rules/Class_/MustBeUsedTraitRule.php new file mode 100644 index 00000000..eca51606 --- /dev/null +++ b/src/Rule/Rules/Class_/MustBeUsedTraitRule.php @@ -0,0 +1,68 @@ +isTrait) { + return false; + } + + if (! $classNode->isInLayer($this->layer)) { + return false; + } + + if ($this->classNamePattern !== null) { + return $classNode->nameMatches($this->classNamePattern, isFullName: true); + } + + return true; + } + + public function evaluate(ClassNode $classNode): ?RuleViolation + { + if ($classNode->isUsed) { + return null; + } + + return new RuleViolation( + message: sprintf( + 'Trait [%s] must be used by a class, trait, or enum', + $classNode->className + ), + file: $classNode->file, + line: $classNode->line, + className: $classNode->className, + layer: $classNode->layer, + ); + } + + protected function createFixerVisitor(RuleViolation $ruleViolation): RemoveClassLikeVisitor + { + return new RemoveClassLikeVisitor($ruleViolation->className); + } + + protected function shouldRemoveFileWhenEmpty(): bool + { + return true; + } +} diff --git a/src/Rule/UsedTraitAwareRuleInterface.php b/src/Rule/UsedTraitAwareRuleInterface.php new file mode 100644 index 00000000..40e60bbc --- /dev/null +++ b/src/Rule/UsedTraitAwareRuleInterface.php @@ -0,0 +1,18 @@ +isUsed. + * + * Trade-off: only usage within the scanned paths is known. A trait used solely + * by a consumer outside the scan is reported as if not used. + */ +interface UsedTraitAwareRuleInterface extends RuleInterface +{ +} diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index 1811639a..a5ad362c 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -19,6 +19,7 @@ use Boundwize\StructArmed\Preset\Presets\Psr15Preset; use Boundwize\StructArmed\Preset\Presets\Psr1Preset; use Boundwize\StructArmed\Preset\Presets\Psr4Preset; +use Boundwize\StructArmed\Preset\Presets\YagniPreset; use Boundwize\StructArmed\Progress\ProgressHandlerInterface; use Boundwize\StructArmed\Rule\FileAnalysisRuleInterface; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeFinalRule; @@ -312,6 +313,167 @@ public function testMustBeFinalRuleFlagsExtendedClassWhenChildIsOutsideScannedPa $this->assertSame('App\BaseHandler', $violations[0]->className); } + public function testYagniPresetReportsOnlyUnusedAbstractions(): void + { + $consumer = 'makeTempProject([ + 'src/UsedInterface.php' => ' ' ' ' ' ' ' ' $consumer, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + $interfaceViolations = $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED); + $abstractViolations = $ruleViolationCollection->forRule(YagniPreset::ABSTRACT_CLASS_MUST_BE_OVERRIDDEN); + $traitViolations = $ruleViolationCollection->forRule(YagniPreset::TRAIT_MUST_BE_USED); + + // BaseInterface is extended by ChildInterface, so only UnusedInterface + // and the never-implemented ChildInterface itself are reported. + $interfaceClassNames = array_map( + static fn (RuleViolation $ruleViolation): string => $ruleViolation->className, + $interfaceViolations + ); + sort($interfaceClassNames); + + $this->assertSame(['App\ChildInterface', 'App\UnusedInterface'], $interfaceClassNames); + + $this->assertCount(1, $abstractViolations); + $this->assertSame('App\UnusedBase', $abstractViolations[0]->className); + + $this->assertCount(1, $traitViolations); + $this->assertSame('App\UnusedTrait', $traitViolations[0]->className); + } + + public function testMustBeImplementedInterfaceRuleRecognizesTransitiveImplementation(): void + { + $basePath = $this->makeTempProject([ + 'src/BaseInterface.php' => ' ' 'withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + // Consumer implements ChildInterface, which transitively implements + // BaseInterface — neither interface is speculative. + $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED)); + } + + public function testYagniRulesDoNotFlagAbstractionsUsedByAnonymousClass(): void + { + $factory = 'makeTempProject([ + 'src/Contract.php' => ' ' $factory, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED)); + $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::TRAIT_MUST_BE_USED)); + } + + public function testYagniRulesDoNotFlagAnonymousClassUsageOnCachedRun(): void + { + $factory = 'makeTempProject([ + 'src/Contract.php' => ' ' $factory, + ]); + $analysisResultCache = new AnalysisResultCache($basePath, new FileHashProvider(), 'cache'); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + $warmViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + // The anonymous-class implements/use lists must survive the class-node + // cache round-trip. + $this->assertFalse($ruleViolationCollection->hasViolations()); + $this->assertFalse($warmViolationCollection->hasViolations()); + } + + public function testYagniRulesFollowOriginalNamesWhenUsageIsAliased(): void + { + $consumer = 'makeTempProject([ + 'src/BaseHandler.php' => ' ' ' $consumer, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + // Aliased imports resolve to the original names, so the abstractions + // count as used. + $this->assertFalse($ruleViolationCollection->hasViolations()); + } + + public function testYagniRulesRecognizeUsageWithParallelRunner(): void + { + $consumer = 'makeTempProject([ + 'src/UsedInterface.php' => ' ' ' $consumer, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::parallel()); + + $this->assertFalse($ruleViolationCollection->hasViolations()); + } + public function testMustBeFinalRuleReportsSameExtendedClassesOnCachedRun(): void { $basePath = $this->makeTempProject([ diff --git a/tests/Analyser/ClassNodeTest.php b/tests/Analyser/ClassNodeTest.php index 17eb053b..4f763a3a 100644 --- a/tests/Analyser/ClassNodeTest.php +++ b/tests/Analyser/ClassNodeTest.php @@ -269,6 +269,53 @@ className: 'App\\Domain\\OrderService', $this->assertFalse($classNode->isExtended); } + public function testSetImplementedTogglesIsImplementedFlag(): void + { + $classNode = new ClassNode( + className: 'App\\Domain\\OrderRepositoryInterface', + file: '/src/OrderRepositoryInterface.php', + line: 5, + layer: 'Domain', + extends: null, + isAbstract: false, + isFinal: false, + isInterface: true, + isReadonly: false, + ); + + $this->assertFalse($classNode->isImplemented); + + $classNode->setImplemented(true); + $this->assertTrue($classNode->isImplemented); + + $classNode->setImplemented(false); + $this->assertFalse($classNode->isImplemented); + } + + public function testSetUsedTogglesIsUsedFlag(): void + { + $classNode = new ClassNode( + className: 'App\\Domain\\TimestampableTrait', + file: '/src/TimestampableTrait.php', + line: 5, + layer: 'Domain', + extends: null, + isAbstract: false, + isFinal: false, + isInterface: false, + isReadonly: false, + isTrait: true, + ); + + $this->assertFalse($classNode->isUsed); + + $classNode->setUsed(true); + $this->assertTrue($classNode->isUsed); + + $classNode->setUsed(false); + $this->assertFalse($classNode->isUsed); + } + public function testDependsOnMatchesExistingClassesExactly(): void { $classNode = new ClassNode( diff --git a/tests/Cache/AnalysisResultCacheTest.php b/tests/Cache/AnalysisResultCacheTest.php index 97f07db6..268e4dc8 100644 --- a/tests/Cache/AnalysisResultCacheTest.php +++ b/tests/Cache/AnalysisResultCacheTest.php @@ -603,9 +603,11 @@ public function testStoresAndLoadsAnonymousClassNodes(): void $classNodes = [$this->makeClassNode($sourceFile)]; $anonymousClassNodes = [ new AnonymousClassNode( - file: $sourceFile, - line: 7, - extends: 'App\BaseHandler', + file: $sourceFile, + line: 7, + extends: 'App\BaseHandler', + implements: ['App\Contract'], + traits: ['App\Helper'], ), ]; @@ -675,6 +677,12 @@ public static function corruptedAnonymousClassNodesProvider(): Iterator yield 'not an array' => ['invalid']; yield 'entry not an array' => [['invalid']]; yield 'entry with invalid field types' => [[['file' => 1, 'line' => 'x', 'extends' => null]]]; + yield 'entry with invalid implements' => [ + [['file' => '/Foo.php', 'line' => 7, 'extends' => null, 'implements' => ['App\Contract', 1]]], + ]; + yield 'entry with invalid traits' => [ + [['file' => '/Foo.php', 'line' => 7, 'extends' => null, 'traits' => 'invalid']], + ]; } #[DataProvider('corruptedAnonymousClassNodesProvider')] diff --git a/tests/Cli/InitCommandTest.php b/tests/Cli/InitCommandTest.php index 8f08f368..2411e8b7 100644 --- a/tests/Cli/InitCommandTest.php +++ b/tests/Cli/InitCommandTest.php @@ -69,6 +69,11 @@ public static function presetProvider(): iterable ' ->withPreset(Preset::PSR4());', ]; + yield 'yagni' => [ + ['--preset=yagni'], + ' ->withPreset(Preset::YAGNI());', + ]; + yield 'all' => [ ['--preset=all'], " ->withPresets(\n" @@ -77,7 +82,8 @@ public static function presetProvider(): iterable . " Preset::PSR15(),\n" . " Preset::PSR4(),\n" . " Preset::DDD(),\n" - . " Preset::MVC()\n" + . " Preset::MVC(),\n" + . " Preset::YAGNI()\n" . " );", ]; } diff --git a/tests/Cli/StructArmedApplicationCommandRoutingTest.php b/tests/Cli/StructArmedApplicationCommandRoutingTest.php index fff2415a..b1df5d94 100644 --- a/tests/Cli/StructArmedApplicationCommandRoutingTest.php +++ b/tests/Cli/StructArmedApplicationCommandRoutingTest.php @@ -35,7 +35,7 @@ public function testApplicationPrintsUsageWithoutCommand(): void $this->assertSame(0, $exitCode); $this->assertStringContainsString('structarmed --version', $output); $this->assertStringContainsString( - 'structarmed init [--preset=ddd|mvc|psr1|psr12|psr15|psr4|all]', + 'structarmed init [--preset=ddd|mvc|psr1|psr12|psr15|psr4|yagni|all]', $output ); $this->assertStringContainsString('structarmed analyse|analyze', $output); diff --git a/tests/Cli/StructArmedApplicationTest.php b/tests/Cli/StructArmedApplicationTest.php index b6b8c8d3..78fc9af8 100644 --- a/tests/Cli/StructArmedApplicationTest.php +++ b/tests/Cli/StructArmedApplicationTest.php @@ -172,6 +172,11 @@ public static function presetProvider(): iterable ' ->withPreset(Preset::PSR4());', ]; + yield 'yagni' => [ + ['--preset=yagni'], + ' ->withPreset(Preset::YAGNI());', + ]; + yield 'all' => [ ['--preset=all'], " ->withPresets(\n" @@ -180,7 +185,8 @@ public static function presetProvider(): iterable . " Preset::PSR15(),\n" . " Preset::PSR4(),\n" . " Preset::DDD(),\n" - . " Preset::MVC()\n" + . " Preset::MVC(),\n" + . " Preset::YAGNI()\n" . " );", ]; } diff --git a/tests/Preset/PresetTest.php b/tests/Preset/PresetTest.php index 05c2c439..e2693757 100644 --- a/tests/Preset/PresetTest.php +++ b/tests/Preset/PresetTest.php @@ -13,6 +13,10 @@ use Boundwize\StructArmed\Preset\Presets\Psr1Preset; use Boundwize\StructArmed\Preset\Presets\Psr4Preset; use Boundwize\StructArmed\Preset\Presets\ResolvesSourceLayerNameTrait; +use Boundwize\StructArmed\Preset\Presets\YagniPreset; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeImplementedInterfaceRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeOverriddenAbstractClassRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedTraitRule; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; @@ -24,8 +28,44 @@ #[CoversClass(Psr15Preset::class)] #[CoversClass(Psr4Preset::class)] #[CoversClass(ResolvesSourceLayerNameTrait::class)] +#[CoversClass(YagniPreset::class)] final class PresetTest extends TestCase { + public function testYagniPresetRegistersSourceLayerAndRules(): void + { + $architecture = Architecture::define(); + + Preset::YAGNI( + sourcePaths: ['src/'], + )->apply($architecture); + + $this->assertSame(['Source' => ['src/']], $architecture->getLayers()); + + $rules = $architecture->getRules(); + $this->assertInstanceOf( + MustBeImplementedInterfaceRule::class, + $rules[YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED] ?? null + ); + $this->assertInstanceOf( + MustBeOverriddenAbstractClassRule::class, + $rules[YagniPreset::ABSTRACT_CLASS_MUST_BE_OVERRIDDEN] ?? null + ); + $this->assertInstanceOf( + MustBeUsedTraitRule::class, + $rules[YagniPreset::TRAIT_MUST_BE_USED] ?? null + ); + } + + public function testYagniPresetUsesComposerSourcePathsByDefault(): void + { + $architecture = Architecture::define(); + + Preset::YAGNI()->apply($architecture); + + // A null source path list defers to Composer-discovered PSR-4 paths. + $this->assertSame(['Source' => []], $architecture->getLayers()); + } + public function testPsr1PresetRegistersSourceLayerAndRules(): void { $architecture = Architecture::define(); diff --git a/tests/Rule/Class_/MustBeImplementedInterfaceRuleTest.php b/tests/Rule/Class_/MustBeImplementedInterfaceRuleTest.php new file mode 100644 index 00000000..09115c7c --- /dev/null +++ b/tests/Rule/Class_/MustBeImplementedInterfaceRuleTest.php @@ -0,0 +1,213 @@ +makeNode(isImplemented: true); + + $this->assertNotInstanceOf( + RuleViolation::class, + $mustBeImplementedInterfaceRule->evaluate($classNode) + ); + } + + public function testViolatesWhenInterfaceIsNotImplemented(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isImplemented: false); + + $violation = $mustBeImplementedInterfaceRule->evaluate($classNode); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertStringContainsString('must be implemented', $violation->message); + } + + public function testIsImplementedInterfaceAware(): void + { + $this->assertInstanceOf( + ImplementedInterfaceAwareRuleInterface::class, + new MustBeImplementedInterfaceRule(layer: 'Domain') + ); + } + + public function testIsFixable(): void + { + $this->assertInstanceOf(FixableInterface::class, new MustBeImplementedInterfaceRule(layer: 'Domain')); + } + + public function testCreatesRemoveClassLikeFixerVisitor(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $reflectionMethod = new ReflectionMethod($mustBeImplementedInterfaceRule, 'createFixerVisitor'); + $removeClassLikeVisitor = $reflectionMethod->invoke( + $mustBeImplementedInterfaceRule, + new RuleViolation( + message: 'Interface [App\\Unused] must be implemented by a class or extended by another interface', + file: '/src/Unused.php', + line: 1, + className: 'App\\Unused', + layer: 'Domain', + ) + ); + + $this->assertInstanceOf(RemoveClassLikeVisitor::class, $removeClassLikeVisitor); + } + + public function testDoesNotApplyToWrongLayer(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(layer: 'Infrastructure'); + + $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToClasses(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isInterface: false); + + $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToTraits(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isInterface: false, isTrait: true); + + $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + } + + public function testAppliesToLayerWhenNoPatternConfigured(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(); + + $this->assertTrue($mustBeImplementedInterfaceRule->appliesTo($classNode)); + } + + public function testAppliesToMatchingPattern(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule( + layer: 'Domain', + classNamePattern: '/Interface$/' + ); + $classNode = $this->makeNode(className: 'App\\Domain\\OrderRepositoryInterface'); + + $this->assertTrue($mustBeImplementedInterfaceRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToNonMatchingPattern(): void + { + $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule( + layer: 'Domain', + classNamePattern: '/Repository$/' + ); + $classNode = $this->makeNode(className: 'App\\Domain\\OrderRepositoryInterface'); + + $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + } + + public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); + $file = $temporaryDirectory . '/UnusedInterface.php'; + + file_put_contents( + $file, + "assertTrue($mustBeImplementedInterfaceRule->fix(new RuleViolation( + message: 'Interface [App\\UnusedInterface] must be implemented by a class' + . ' or extended by another interface', + file: $file, + line: 7, + className: 'App\\UnusedInterface', + layer: 'Domain', + ))); + $this->assertFileDoesNotExist($file); + } + + public function testFixKeepsFileWhenOtherCodeRemains(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); + $file = $temporaryDirectory . '/Contracts.php'; + + file_put_contents( + $file, + "assertTrue($mustBeImplementedInterfaceRule->fix(new RuleViolation( + message: 'Interface [App\\UnusedInterface] must be implemented by a class' + . ' or extended by another interface', + file: $file, + line: 7, + className: 'App\\UnusedInterface', + layer: 'Domain', + ))); + $this->assertFileExists($file); + + $fixedCode = (string) file_get_contents($file); + + $this->assertStringNotContainsString('interface UnusedInterface', $fixedCode); + $this->assertStringContainsString('final class Order', $fixedCode); + } +} diff --git a/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php b/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php new file mode 100644 index 00000000..7ad3261a --- /dev/null +++ b/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php @@ -0,0 +1,190 @@ +makeNode(isExtended: true); + + $this->assertNotInstanceOf( + RuleViolation::class, + $mustBeOverriddenAbstractClassRule->evaluate($classNode) + ); + } + + public function testViolatesWhenAbstractClassIsNotExtended(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isExtended: false); + + $violation = $mustBeOverriddenAbstractClassRule->evaluate($classNode); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertStringContainsString('must be extended', $violation->message); + } + + public function testIsExtendedClassAware(): void + { + $this->assertInstanceOf( + ExtendedClassAwareRuleInterface::class, + new MustBeOverriddenAbstractClassRule(layer: 'Domain') + ); + } + + public function testIsFixable(): void + { + $this->assertInstanceOf(FixableInterface::class, new MustBeOverriddenAbstractClassRule(layer: 'Domain')); + } + + public function testCreatesRemoveClassLikeFixerVisitor(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $reflectionMethod = new ReflectionMethod( + $mustBeOverriddenAbstractClassRule, + 'createFixerVisitor' + ); + $removeClassLikeVisitor = $reflectionMethod->invoke( + $mustBeOverriddenAbstractClassRule, + new RuleViolation( + message: 'Abstract class [App\\AbstractHandler] must be extended by a class', + file: '/src/AbstractHandler.php', + line: 1, + className: 'App\\AbstractHandler', + layer: 'Domain', + ) + ); + + $this->assertInstanceOf(RemoveClassLikeVisitor::class, $removeClassLikeVisitor); + } + + public function testDoesNotApplyToWrongLayer(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(layer: 'Infrastructure'); + + $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToConcreteClasses(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isAbstract: false); + + $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToInterfaces(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isInterface: true); + + $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToTraits(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isTrait: true); + + $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + } + + public function testAppliesToLayerWhenNoPatternConfigured(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(); + + $this->assertTrue($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + } + + public function testAppliesToMatchingPattern(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule( + layer: 'Domain', + classNamePattern: '/^App\\\\Domain\\\\Abstract/' + ); + $classNode = $this->makeNode(); + + $this->assertTrue($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToNonMatchingPattern(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule( + layer: 'Domain', + classNamePattern: '/Base$/' + ); + $classNode = $this->makeNode(); + + $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + } + + public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-abstract'); + $file = $temporaryDirectory . '/AbstractHandler.php'; + + file_put_contents( + $file, + "assertTrue($mustBeOverriddenAbstractClassRule->fix(new RuleViolation( + message: 'Abstract class [App\\AbstractHandler] must be extended by a class', + file: $file, + line: 7, + className: 'App\\AbstractHandler', + layer: 'Domain', + ))); + $this->assertFileDoesNotExist($file); + } +} diff --git a/tests/Rule/Class_/MustBeUsedTraitRuleTest.php b/tests/Rule/Class_/MustBeUsedTraitRuleTest.php new file mode 100644 index 00000000..a8ec27e0 --- /dev/null +++ b/tests/Rule/Class_/MustBeUsedTraitRuleTest.php @@ -0,0 +1,169 @@ +makeNode(isUsed: true); + + $this->assertNotInstanceOf(RuleViolation::class, $mustBeUsedTraitRule->evaluate($classNode)); + } + + public function testViolatesWhenTraitIsNotUsed(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); + $classNode = $this->makeNode(isUsed: false); + + $violation = $mustBeUsedTraitRule->evaluate($classNode); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertStringContainsString('must be used', $violation->message); + } + + public function testIsUsedTraitAware(): void + { + $this->assertInstanceOf( + UsedTraitAwareRuleInterface::class, + new MustBeUsedTraitRule(layer: 'Domain') + ); + } + + public function testIsFixable(): void + { + $this->assertInstanceOf(FixableInterface::class, new MustBeUsedTraitRule(layer: 'Domain')); + } + + public function testCreatesRemoveClassLikeFixerVisitor(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); + $reflectionMethod = new ReflectionMethod($mustBeUsedTraitRule, 'createFixerVisitor'); + $removeClassLikeVisitor = $reflectionMethod->invoke( + $mustBeUsedTraitRule, + new RuleViolation( + message: 'Trait [App\\UnusedTrait] must be used by a class, trait, or enum', + file: '/src/UnusedTrait.php', + line: 1, + className: 'App\\UnusedTrait', + layer: 'Domain', + ) + ); + + $this->assertInstanceOf(RemoveClassLikeVisitor::class, $removeClassLikeVisitor); + } + + public function testDoesNotApplyToWrongLayer(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); + $classNode = $this->makeNode(layer: 'Infrastructure'); + + $this->assertFalse($mustBeUsedTraitRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToClasses(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); + $classNode = $this->makeNode(isTrait: false); + + $this->assertFalse($mustBeUsedTraitRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToInterfaces(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); + $classNode = $this->makeNode(isInterface: true, isTrait: false); + + $this->assertFalse($mustBeUsedTraitRule->appliesTo($classNode)); + } + + public function testAppliesToLayerWhenNoPatternConfigured(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); + $classNode = $this->makeNode(); + + $this->assertTrue($mustBeUsedTraitRule->appliesTo($classNode)); + } + + public function testAppliesToMatchingPattern(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain', classNamePattern: '/Trait$/'); + $classNode = $this->makeNode(); + + $this->assertTrue($mustBeUsedTraitRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToNonMatchingPattern(): void + { + $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain', classNamePattern: '/Helper$/'); + $classNode = $this->makeNode(); + + $this->assertFalse($mustBeUsedTraitRule->appliesTo($classNode)); + } + + public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-trait'); + $file = $temporaryDirectory . '/UnusedTrait.php'; + + file_put_contents( + $file, + "assertTrue($mustBeUsedTraitRule->fix(new RuleViolation( + message: 'Trait [App\\UnusedTrait] must be used by a class, trait, or enum', + file: $file, + line: 7, + className: 'App\\UnusedTrait', + layer: 'Domain', + ))); + $this->assertFileDoesNotExist($file); + } +} diff --git a/tests/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitorTest.php b/tests/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitorTest.php new file mode 100644 index 00000000..07bf6803 --- /dev/null +++ b/tests/Rule/Fixer/PhpParser/ClassLike/RemoveClassLikeVisitorTest.php @@ -0,0 +1,80 @@ +namespacedName = new Name('App\\UnusedInterface'); + + $statements = (new NodeTraverser(new RemoveClassLikeVisitor('App\\UnusedInterface'))) + ->traverse([$interface]); + + $this->assertSame([], $statements); + } + + public function testRemovesMatchingAbstractClass(): void + { + $class = new Class_('AbstractHandler'); + $class->namespacedName = new Name('App\\AbstractHandler'); + + $statements = (new NodeTraverser(new RemoveClassLikeVisitor('App\\AbstractHandler'))) + ->traverse([$class]); + + $this->assertSame([], $statements); + } + + public function testRemovesMatchingTrait(): void + { + $trait = new Trait_('UnusedTrait'); + $trait->namespacedName = new Name('App\\UnusedTrait'); + + $statements = (new NodeTraverser(new RemoveClassLikeVisitor('App\\UnusedTrait'))) + ->traverse([$trait]); + + $this->assertSame([], $statements); + } + + public function testKeepsNonMatchingClassLike(): void + { + $interface = new Interface_('UsedInterface'); + $interface->namespacedName = new Name('App\\UsedInterface'); + + $statements = (new NodeTraverser(new RemoveClassLikeVisitor('App\\UnusedInterface'))) + ->traverse([$interface]); + + $this->assertSame([$interface], $statements); + } + + public function testKeepsAnonymousClass(): void + { + $class = new Class_(null); + + $statements = (new NodeTraverser(new RemoveClassLikeVisitor('App\\UnusedInterface'))) + ->traverse([$class]); + + $this->assertSame([$class], $statements); + } + + public function testDoesNotRemoveNonClassLikeNode(): void + { + $removeClassLikeVisitor = new RemoveClassLikeVisitor('App\\UnusedInterface'); + + $this->assertNull($removeClassLikeVisitor->leaveNode(new ClassMethod('save'))); + } +} From dff6c03ec890c866bfda2848f21fa8508299f0a9 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 02:36:58 +0700 Subject: [PATCH 02/19] enable YAGNI preset --- structarmed.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/structarmed.php b/structarmed.php index a96c55f6..df043115 100644 --- a/structarmed.php +++ b/structarmed.php @@ -52,4 +52,4 @@ __DIR__ . '/tests/Analyser/Parallel/MockFunctions.php', ], ]) - ->withPresets(Preset::PSR1(), Preset::PSR12(), Preset::PSR4()); + ->withPresets(Preset::PSR1(), Preset::PSR12(), Preset::PSR4(), Preset::YAGNI()); From 6409136406a8627f61f59d5612979d268a919de1 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 02:38:10 +0700 Subject: [PATCH 03/19] enable MustBeFinalRule --- structarmed.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/structarmed.php b/structarmed.php index df043115..baa6adcd 100644 --- a/structarmed.php +++ b/structarmed.php @@ -5,6 +5,7 @@ use Boundwize\StructArmed\Architecture; use Boundwize\StructArmed\Preset\Preset; use Boundwize\StructArmed\Preset\Presets\Psr1Preset; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeFinalRule; return Architecture::define() ->layer('Analyser', 'src/Analyser/') @@ -52,4 +53,8 @@ __DIR__ . '/tests/Analyser/Parallel/MockFunctions.php', ], ]) - ->withPresets(Preset::PSR1(), Preset::PSR12(), Preset::PSR4(), Preset::YAGNI()); + ->withPresets(Preset::PSR1(), Preset::PSR12(), Preset::PSR4(), Preset::YAGNI()) + ->rule( + 'source.must_be_final', + new MustBeFinalRule(layer: 'Source') + ); From 904dade0177dd49743c3af6982949bb632cbf2f8 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 02:47:16 +0700 Subject: [PATCH 04/19] ensure safe fix --- src/Cli/AnalyseCommand.php | 20 +++++++- tests/Cli/StructArmedApplicationTest.php | 63 ++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/Cli/AnalyseCommand.php b/src/Cli/AnalyseCommand.php index db32f331..c3d79a82 100644 --- a/src/Cli/AnalyseCommand.php +++ b/src/Cli/AnalyseCommand.php @@ -59,6 +59,13 @@ '--fix' => 'fix', ]; + /** + * Upper bound on fix/re-analyse passes. Fixers converge naturally (each + * pass removes or rewrites code), so this only guards against a fixer that + * keeps reporting success without resolving its violation. + */ + private const MAX_FIX_PASSES = 10; + public function __construct(private ?ProgressHandlerInterface $progressHandler = null) { } @@ -172,9 +179,18 @@ public function run(array $arguments, string $basePath): int } if (isset($options['fix'])) { - $fixedCount = $this->fixViolations($architecture, $ruleViolationCollection); + // Removal fixers can cascade: deleting an unused child abstraction + // may leave its parent unused, so fix and re-analyse until a pass + // fixes nothing. The pass cap only guards against a fixer that + // reports success without resolving its violation. + for ($fixPass = 0; $fixPass < self::MAX_FIX_PASSES; $fixPass++) { + $passFixedCount = $this->fixViolations($architecture, $ruleViolationCollection); + + if ($passFixedCount === 0) { + break; + } - if ($fixedCount > 0) { + $fixedCount += $passFixedCount; $analysisResultCache->clear(); $files = $analyser->filesForAnalysis($architecture, $scanPaths); diff --git a/tests/Cli/StructArmedApplicationTest.php b/tests/Cli/StructArmedApplicationTest.php index 78fc9af8..9292b847 100644 --- a/tests/Cli/StructArmedApplicationTest.php +++ b/tests/Cli/StructArmedApplicationTest.php @@ -576,6 +576,69 @@ public function testAnalyseCommandFixesFixableViolations(): void } } + public function testAnalyseCommandFixesCascadingYagniViolationsInOneRun(): void + { + $basePath = $this->createProjectDirectory(); + + // ChildInterface keeps BaseInterface "used" until the fixer removes it, + // which makes BaseInterface newly unused — a single --fix run must keep + // fixing until no violations remain. + file_put_contents($basePath . '/src/BaseInterface.php', <<<'PHP' +withPreset(Preset::YAGNI(sourcePaths: ['src/'])); +PHP); + + try { + [$exitCode, $output] = $this->runApplication( + [ + 'structarmed', + 'analyze', + '--config=' . $basePath . '/structarmed.php', + '--fix', + '--no-progress', + ], + $basePath + ); + + $this->assertSame(0, $exitCode, $output); + $this->assertStringContainsString('2 violations have been fixed.', $this->withoutAnsi($output)); + $this->assertStringContainsString('No violations found', $output); + $this->assertFileDoesNotExist($basePath . '/src/ChildInterface.php'); + $this->assertFileDoesNotExist($basePath . '/src/BaseInterface.php'); + } finally { + $this->removeTempDirectory($basePath); + } + } + public function testAnalyseCommandTracksComposerJsonProgressAsSingleFile(): void { $basePath = $this->createProjectDirectoryWithMissingComposerPsr4Path(); From 7701f5a8a319144c2e2318547e8b02bead4f05ef Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 03:02:11 +0700 Subject: [PATCH 05/19] handle referenced --- docs/available-rules.md | 6 +-- src/Analyser/Analyser.php | 27 ++++++++--- src/Analyser/ClassNode.php | 7 +-- src/Cli/AnalyseCommand.php | 6 ++- .../PhpParser/PhpParserFixerProcessor.php | 13 +++++- .../Class_/MustBeImplementedInterfaceRule.php | 6 +++ .../MustBeOverriddenAbstractClassRule.php | 6 +++ tests/Analyser/AnalyserTest.php | 46 +++++++++++++++++++ .../MustBeImplementedInterfaceRuleTest.php | 46 ++++++++++++++++++- .../MustBeOverriddenAbstractClassRuleTest.php | 13 ++++++ 10 files changed, 158 insertions(+), 18 deletions(-) diff --git a/docs/available-rules.md b/docs/available-rules.md index 284e3f0f..65259195 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -81,10 +81,10 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | -| `MustBeImplementedInterfaceRule` | `new MustBeImplementedInterfaceRule(layer: 'Source')` | Interfaces are implemented by a scanned class (directly or through inheritance) or extended by another scanned interface. Supports `--fix` by removing the unused interface (and deleting its file when only boilerplate remains). | +| `MustBeImplementedInterfaceRule` | `new MustBeImplementedInterfaceRule(layer: 'Source')` | Interfaces are implemented by a scanned class (directly or through inheritance), extended by another scanned interface, or referenced as a dependency (type hint, `instanceof`, `::class`, ...). Supports `--fix` by removing the unused interface (and deleting its file when only boilerplate remains). | | `MustBeInterfaceRule` | `new MustBeInterfaceRule(layer: 'Contract', classNamePattern: '/Interface$/')` | Matching declarations in a layer are interfaces. | -| `MustBeOverriddenAbstractClassRule` | `new MustBeOverriddenAbstractClassRule(layer: 'Source')` | Abstract classes are extended by a scanned class. Supports `--fix` by removing the unused abstract class (and deleting its file when only boilerplate remains). | -| `MustBeUsedTraitRule` | `new MustBeUsedTraitRule(layer: 'Source')` | Traits are used by a scanned class, trait, or enum. Supports `--fix` by removing the unused trait (and deleting its file when only boilerplate remains). | +| `MustBeOverriddenAbstractClassRule` | `new MustBeOverriddenAbstractClassRule(layer: 'Source')` | Abstract classes are extended by a scanned class or referenced as a dependency (type hint, `instanceof`, `::class`, static call, ...). Supports `--fix` by removing the unused abstract class (and deleting its file when only boilerplate remains). | +| `MustBeUsedTraitRule` | `new MustBeUsedTraitRule(layer: 'Source')` | Traits are used by a scanned class, trait, or enum, or referenced as a dependency (`::class`, static call, ...). Supports `--fix` by removing the unused trait (and deleting its file when only boilerplate remains). | | `MustDeclareConstantVisibilityRule` | `new MustDeclareConstantVisibilityRule(layer: 'Source')` | Class constants declare `public`, `protected`, or `private`. Supports `--fix`. | | `MustDeclareMethodVisibilityRule` | `new MustDeclareMethodVisibilityRule(layer: 'Source')` | Methods declare `public`, `protected`, or `private`. Supports `--fix`. | | `MustDeclarePropertyVisibilityRule` | `new MustDeclarePropertyVisibilityRule(layer: 'Source')` | Properties declare `public`, `protected`, or `private`. Supports `--fix`. | diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index b3c2c4ef..0e64f604 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -168,8 +168,8 @@ public function analyse( $this->markImplementedInterfaces($classNodes, $extractionResult); } - if ($hasUsedTraitAwareRule) { - $this->markUsedTraits($classNodes, $extractionResult); + if ($hasExtendedClassAwareRule || $hasImplementedInterfaceAwareRule || $hasUsedTraitAwareRule) { + $this->markUsedClassLikes($classNodes, $extractionResult); } if ($withFileAnalysis) { @@ -743,14 +743,17 @@ private function markImplementedInterfaces(array $classNodes, ExtractionResult $ } /** - * Flag every trait that another scanned class-like (class, trait, or enum) - * uses. Trait usage is a direct declaration, so no recursive chain is - * needed: a trait used only by another unused trait stays flagged as used - * until that trait is removed, at which point the next run reports it. + * Flag every class-like that another scanned class-like (class, trait, or + * enum) uses — as a trait, or by referencing it as a dependency: a type + * hint, an instanceof check, a ::class constant, a static call, and so on. + * Self-references are ignored: a class-like cannot keep itself alive. + * Usage is a direct declaration, so no recursive chain is needed: a + * class-like used only by another unused one stays flagged as used until + * its user is removed, at which point the next run reports it. * * @param list $classNodes */ - private function markUsedTraits(array $classNodes, ExtractionResult $extractionResult): void + private function markUsedClassLikes(array $classNodes, ExtractionResult $extractionResult): void { $used = []; @@ -758,6 +761,16 @@ private function markUsedTraits(array $classNodes, ExtractionResult $extractionR foreach ($classNode->traits as $trait) { $used[strtolower($trait)] = true; } + + $selfKey = strtolower($classNode->className); + + foreach ($classNode->dependencies as $dependency) { + $dependencyKey = strtolower($dependency); + + if ($dependencyKey !== $selfKey) { + $used[$dependencyKey] = true; + } + } } // Anonymous classes (`new class { use Foo; }`) have no ClassNode of diff --git a/src/Analyser/ClassNode.php b/src/Analyser/ClassNode.php index 45b8bb6a..a03a62aa 100644 --- a/src/Analyser/ClassNode.php +++ b/src/Analyser/ClassNode.php @@ -97,9 +97,10 @@ public function setImplemented(bool $isImplemented): void } /** - * Whether another scanned class-like uses this trait. Computed by the - * analyser for rules implementing UsedTraitAwareRuleInterface; false - * otherwise. + * Whether another scanned class-like uses this class-like — as a trait, or + * by referencing it as a dependency (type hint, instanceof, ::class, + * static call, ...). Computed by the analyser when a usage-aware rule is + * active; false otherwise. */ public function setUsed(bool $isUsed): void { diff --git a/src/Cli/AnalyseCommand.php b/src/Cli/AnalyseCommand.php index c3d79a82..f3d82dc6 100644 --- a/src/Cli/AnalyseCommand.php +++ b/src/Cli/AnalyseCommand.php @@ -61,8 +61,10 @@ /** * Upper bound on fix/re-analyse passes. Fixers converge naturally (each - * pass removes or rewrites code), so this only guards against a fixer that - * keeps reporting success without resolving its violation. + * pass removes or rewrites code), so this mainly guards against a fixer + * that keeps reporting success without resolving its violation. A cascade + * deeper than this cap (e.g. an unused-abstraction chain of more than + * ten levels) needs another --fix invocation to finish. */ private const MAX_FIX_PASSES = 10; diff --git a/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php b/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php index 4e0c77f5..e69db504 100644 --- a/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php +++ b/src/Rule/Fixer/PhpParser/PhpParserFixerProcessor.php @@ -67,9 +67,18 @@ public function process(string $file, NodeVisitor $nodeVisitor, bool $removeFile private function hasOnlyDeclarations(array $statements): bool { foreach ($statements as $statement) { + // The block form `declare(...) { ... }` carries statements of its + // own, so only an empty-bodied declare counts as boilerplate. + if ($statement instanceof Declare_) { + if ($statement->stmts !== null && ! $this->hasOnlyDeclarations($statement->stmts)) { + return false; + } + + continue; + } + if ( - $statement instanceof Declare_ - || $statement instanceof Use_ + $statement instanceof Use_ || $statement instanceof GroupUse || $statement instanceof Nop ) { diff --git a/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php b/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php index 0ef41b3c..ad0831b0 100644 --- a/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php +++ b/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php @@ -44,6 +44,12 @@ public function evaluate(ClassNode $classNode): ?RuleViolation return null; } + // A dependency reference (instanceof, type hint, ::class, ...) means + // removing the interface would break the referencing code. + if ($classNode->isUsed) { + return null; + } + return new RuleViolation( message: sprintf( 'Interface [%s] must be implemented by a class or extended by another interface', diff --git a/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php b/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php index 4d695d11..16e8c672 100644 --- a/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php +++ b/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php @@ -44,6 +44,12 @@ public function evaluate(ClassNode $classNode): ?RuleViolation return null; } + // A dependency reference (instanceof, type hint, ::class, static + // call, ...) means removing the class would break the referencing code. + if ($classNode->isUsed) { + return null; + } + return new RuleViolation( message: sprintf( 'Abstract class [%s] must be extended by a class', diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index a5ad362c..e2ab98f1 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -376,6 +376,52 @@ public function testMustBeImplementedInterfaceRuleRecognizesTransitiveImplementa $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED)); } + public function testYagniRulesDoNotFlagAbstractionsReferencedAsDependencies(): void + { + $checker = 'makeTempProject([ + 'src/Contract.php' => ' ' ' $checker, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + // instanceof checks, type hints, and ::class constants are references; + // removing the abstraction would break the referencing code. + $this->assertFalse($ruleViolationCollection->hasViolations()); + } + + public function testYagniRulesIgnoreSelfReferences(): void + { + $basePath = $this->makeTempProject([ + 'src/UnusedInterface.php' => 'withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED); + + // A class-like referencing itself cannot keep itself alive. + $this->assertCount(1, $violations); + $this->assertSame('App\UnusedInterface', $violations[0]->className); + } + public function testYagniRulesDoNotFlagAbstractionsUsedByAnonymousClass(): void { $factory = 'makeNode(isUsed: true); + + $this->assertNotInstanceOf( + RuleViolation::class, + $mustBeImplementedInterfaceRule->evaluate($classNode) + ); + } + public function testViolatesWhenInterfaceIsNotImplemented(): void { $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); @@ -166,7 +179,8 @@ public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void file_put_contents( $file, - "assertFileDoesNotExist($file); } + public function testFixKeepsFileWhenDeclareBlockContainsExecutableCode(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); + $file = $temporaryDirectory . '/ticks.php'; + + // The block form `declare(ticks=1) { ... }` carries executable + // statements — removing the interface must not delete the file. + file_put_contents( + $file, + "assertTrue($mustBeImplementedInterfaceRule->fix(new RuleViolation( + message: 'Interface [UnusedInterface] must be implemented by a class' + . ' or extended by another interface', + file: $file, + line: 7, + className: 'UnusedInterface', + layer: 'Domain', + ))); + $this->assertFileExists($file); + + $fixedCode = (string) file_get_contents($file); + + $this->assertStringNotContainsString('interface UnusedInterface', $fixedCode); + $this->assertStringContainsString("echo 'KEEP ME';", $fixedCode); + } + public function testFixKeepsFileWhenOtherCodeRemains(): void { $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); diff --git a/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php b/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php index 7ad3261a..b928f8e3 100644 --- a/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php +++ b/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php @@ -31,6 +31,7 @@ private function makeNode( bool $isTrait = false, bool $isEnum = false, bool $isExtended = false, + bool $isUsed = false, ): ClassNode { return new ClassNode( className: $className, @@ -45,6 +46,7 @@ className: $className, isTrait: $isTrait, isEnum: $isEnum, isExtended: $isExtended, + isUsed: $isUsed, ); } @@ -59,6 +61,17 @@ public function testPassesWhenAbstractClassIsExtended(): void ); } + public function testPassesWhenAbstractClassIsReferencedAsDependency(): void + { + $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isUsed: true); + + $this->assertNotInstanceOf( + RuleViolation::class, + $mustBeOverriddenAbstractClassRule->evaluate($classNode) + ); + } + public function testViolatesWhenAbstractClassIsNotExtended(): void { $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); From 09a9dcdd9afdc927f9dd7d7f9d91c0c82718ec55 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 03:12:16 +0700 Subject: [PATCH 06/19] handle fileReferences --- src/Analyser/Analyser.php | 27 ++++++- src/Analyser/ClassCollector.php | 35 +++++++++ src/Analyser/ClassNodeExtractor.php | 1 + src/Analyser/ExtractionResult.php | 3 + src/Analyser/Parallel/ClassNodeWorker.php | 2 + .../Parallel/ParallelClassNodeExtractor.php | 21 +++++- src/Cache/AnalysisResultCache.php | 35 ++++++++- tests/Analyser/AnalyserTest.php | 74 +++++++++++++++++++ .../ParallelClassNodeExtractorTest.php | 54 ++++++++++++++ tests/Cache/AnalysisResultCacheTest.php | 30 ++++++++ 10 files changed, 276 insertions(+), 6 deletions(-) diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 0e64f604..002f0f12 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -781,6 +781,15 @@ private function markUsedClassLikes(array $classNodes, ExtractionResult $extract } } + // References made outside any named class-like scope — procedural + // functions, top-level statements, top-level anonymous class bodies — + // have no ClassNode either, so they are tracked per file. + foreach ($extractionResult->fileReferences as $references) { + foreach ($references as $reference) { + $used[strtolower($reference)] = true; + } + } + foreach ($classNodes as $classNode) { if (isset($used[strtolower($classNode->className)])) { $classNode->setUsed(true); @@ -952,6 +961,7 @@ private function collectClassNodes( $classNodes = []; $fileAnalyses = []; $anonymousClassNodes = []; + $fileReferences = []; $filesToParse = []; foreach ($files as $file) { @@ -974,6 +984,10 @@ private function collectClassNodes( $anonymousClassNodes[] = $cachedAnonymousClassNode; } + if ($cachedResult['fileReferences'] !== []) { + $fileReferences[$file] = $cachedResult['fileReferences']; + } + $fileAnalyses[$file] = $cachedResult['fileAnalysis']; continue; @@ -996,6 +1010,10 @@ private function collectClassNodes( foreach ($cachedResult['anonymousClassNodes'] as $cachedAnonymousClassNode) { $anonymousClassNodes[] = $cachedAnonymousClassNode; } + + if ($cachedResult['fileReferences'] !== []) { + $fileReferences[$file] = $cachedResult['fileReferences']; + } } $progressHandler?->start(count($filesToParse)); @@ -1003,7 +1021,7 @@ private function collectClassNodes( if ($filesToParse === []) { $progressHandler?->finish(); - return new ExtractionResult($classNodes, $fileAnalyses, $anonymousClassNodes); + return new ExtractionResult($classNodes, $fileAnalyses, $anonymousClassNodes, $fileReferences); } $options = $analyserOptions ?? AnalyserOptions::parallel(); @@ -1046,6 +1064,10 @@ private function collectClassNodes( $fileAnalyses[$file] = $fileAnalysis; } + foreach ($parsedResult->fileReferences as $file => $parsedFileReferences) { + $fileReferences[$file] = $parsedFileReferences; + } + foreach ($classNodesByFile as $fileToParse => $fileClassNodes) { $this->analysisResultCache?->storeClassNodes( $fileToParse, @@ -1053,12 +1075,13 @@ private function collectClassNodes( $fileClassNodes, $fileAnalyses[$fileToParse] ?? null, $anonymousClassNodesByFile[$fileToParse] ?? [], + $fileReferences[$fileToParse] ?? [], ); } $progressHandler?->finish(); - return new ExtractionResult($classNodes, $fileAnalyses, $anonymousClassNodes); + return new ExtractionResult($classNodes, $fileAnalyses, $anonymousClassNodes, $fileReferences); } /** diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index 2ca9cc0e..b2ea48ae 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -86,6 +86,12 @@ final class ClassCollector extends NodeVisitorAbstract /** @var list */ private array $anonymousClassNodes = []; + /** @var array> */ + private array $fileReferences = []; + + /** @var list */ + private array $currentFileReferences = []; + private string $currentFile = ''; /** @var list */ @@ -120,6 +126,7 @@ public function __construct( public function setCurrentFile(string $file): void { $this->currentFile = $file; + $this->currentFileReferences = []; $this->currentNamespaceUses = []; $this->fileClassLikes = []; $this->fileFunctions = []; @@ -142,6 +149,18 @@ public function getAnonymousClassNodes(): array return $this->anonymousClassNodes; } + /** + * Class-like references made outside any named class-like scope, per file — + * procedural functions, top-level statements, and top-level anonymous + * class bodies. + * + * @return array> + */ + public function getFileReferences(): array + { + return $this->fileReferences; + } + public function enterNode(Node $node): null { if ($node instanceof Namespace_) { @@ -237,6 +256,11 @@ public function afterTraverse(array $nodes): null $this->collectClassLike($fileClassLike); } + if ($this->currentFileReferences !== []) { + $this->fileReferences[$this->currentFile] = array_values(array_unique($this->currentFileReferences)); + $this->currentFileReferences = []; + } + $this->fileClassLikes = []; $this->classLikeAnalysis = []; $this->classLikeMethods = []; @@ -295,6 +319,17 @@ private function finishMethodAnalysis(ClassMethod $classMethod): void private function collectNodeAnalysis(Node $node): void { if ($this->activeClassLikeAnalyses === []) { + // Outside any named class-like scope — procedural functions, + // top-level statements, top-level anonymous class bodies — a + // class-like reference still keeps the referenced class-like alive. + if ($node instanceof FullyQualified) { + $name = $node->toString(); + + if (! isset(self::KEYWORD_CONSTANTS[strtolower($name)])) { + $this->currentFileReferences[] = $name; + } + } + return; } diff --git a/src/Analyser/ClassNodeExtractor.php b/src/Analyser/ClassNodeExtractor.php index ca5c08fd..9182a311 100644 --- a/src/Analyser/ClassNodeExtractor.php +++ b/src/Analyser/ClassNodeExtractor.php @@ -57,6 +57,7 @@ public function extract( $classCollector->getNodes(), $fileAnalyses, $classCollector->getAnonymousClassNodes(), + $classCollector->getFileReferences(), ); } } diff --git a/src/Analyser/ExtractionResult.php b/src/Analyser/ExtractionResult.php index fb51c715..1ac427db 100644 --- a/src/Analyser/ExtractionResult.php +++ b/src/Analyser/ExtractionResult.php @@ -10,11 +10,14 @@ * @param list $classNodes * @param array $fileAnalyses * @param list $anonymousClassNodes + * @param array> $fileReferences Class-like references made outside any + * named class-like scope, per file */ public function __construct( public array $classNodes, public array $fileAnalyses, public array $anonymousClassNodes = [], + public array $fileReferences = [], ) { } } diff --git a/src/Analyser/Parallel/ClassNodeWorker.php b/src/Analyser/Parallel/ClassNodeWorker.php index c4231f4b..6bd3c1c4 100644 --- a/src/Analyser/Parallel/ClassNodeWorker.php +++ b/src/Analyser/Parallel/ClassNodeWorker.php @@ -63,6 +63,7 @@ public static function run(string $inputFile, string $outputFile, mixed $outputS 'nodes' => $result->classNodes, 'fileAnalyses' => $result->fileAnalyses, 'anonymousClassNodes' => $result->anonymousClassNodes, + 'fileReferences' => $result->fileReferences, 'error' => null, ])); @@ -72,6 +73,7 @@ public static function run(string $inputFile, string $outputFile, mixed $outputS 'nodes' => [], 'fileAnalyses' => [], 'anonymousClassNodes' => [], + 'fileReferences' => [], 'error' => sprintf('%s: %s', $throwable::class, $throwable->getMessage()), ])); diff --git a/src/Analyser/Parallel/ParallelClassNodeExtractor.php b/src/Analyser/Parallel/ParallelClassNodeExtractor.php index dcba6a7a..15e14638 100644 --- a/src/Analyser/Parallel/ParallelClassNodeExtractor.php +++ b/src/Analyser/Parallel/ParallelClassNodeExtractor.php @@ -137,6 +137,7 @@ public function extract( $nodes = []; $fileAnalyses = []; $anonymousClassNodes = []; + $fileReferences = []; $failure = null; while ($pending !== []) { @@ -244,6 +245,24 @@ public function extract( $anonymousClassNodes[] = $workerAnonClassNode; } + + $workerFileReferences = $result['fileReferences'] ?? []; + + if (! is_array($workerFileReferences)) { + throw new RuntimeException( + 'Parallel analysis worker returned invalid file references.' + ); + } + + foreach ($workerFileReferences as $file => $references) { + if (! is_string($file) || ! is_array($references)) { + throw new RuntimeException( + 'Parallel analysis worker returned invalid file references.' + ); + } + + $fileReferences[$file] = $references; + } } catch (RuntimeException $runtimeException) { $failure ??= $runtimeException->getMessage(); } finally { @@ -267,7 +286,7 @@ public function extract( throw new RuntimeException($failure); } - return new ExtractionResult($nodes, $fileAnalyses, $anonymousClassNodes); + return new ExtractionResult($nodes, $fileAnalyses, $anonymousClassNodes, $fileReferences); } /** diff --git a/src/Cache/AnalysisResultCache.php b/src/Cache/AnalysisResultCache.php index 822be297..11a4164e 100644 --- a/src/Cache/AnalysisResultCache.php +++ b/src/Cache/AnalysisResultCache.php @@ -186,7 +186,11 @@ private function ensureCacheInitialised(): void } /** - * @return array{classNodes: list, anonymousClassNodes: list}|null + * @return array{ + * classNodes: list, + * anonymousClassNodes: list, + * fileReferences: list + * }|null */ public function loadClassNodes(string $file, string $namespace): ?array { @@ -198,14 +202,16 @@ public function loadClassNodes(string $file, string $namespace): ?array $classNodes = $this->classNodesFromPayload($payload); $anonymousClassNodes = $this->anonymousClassNodesFromPayload($payload); + $fileReferences = $this->fileReferencesFromPayload($payload); - if ($classNodes === null || $anonymousClassNodes === null) { + if ($classNodes === null || $anonymousClassNodes === null || $fileReferences === null) { return null; } return [ 'classNodes' => $classNodes, 'anonymousClassNodes' => $anonymousClassNodes, + 'fileReferences' => $fileReferences, ]; } @@ -213,6 +219,7 @@ public function loadClassNodes(string $file, string $namespace): ?array * @return array{ * classNodes: list, * anonymousClassNodes: list, + * fileReferences: list, * fileAnalysis: FileAnalysis * }|null */ @@ -226,17 +233,24 @@ public function loadClassNodesWithFileAnalysis(string $file, string $namespace): $classNodes = $this->classNodesFromPayload($payload); $anonymousClassNodes = $this->anonymousClassNodesFromPayload($payload); + $fileReferences = $this->fileReferencesFromPayload($payload); $fileAnalysis = is_array($payload['fileAnalysis'] ?? null) ? $this->fileAnalysisFromArray($payload['fileAnalysis']) : null; - if ($classNodes === null || $anonymousClassNodes === null || ! $fileAnalysis instanceof FileAnalysis) { + if ( + $classNodes === null + || $anonymousClassNodes === null + || $fileReferences === null + || ! $fileAnalysis instanceof FileAnalysis + ) { return null; } return [ 'classNodes' => $classNodes, 'anonymousClassNodes' => $anonymousClassNodes, + 'fileReferences' => $fileReferences, 'fileAnalysis' => $fileAnalysis, ]; } @@ -285,6 +299,8 @@ private function classNodesFromPayload(array $payload): ?array /** * @param list $classNodes * @param list $anonymousClassNodes + * @param list $fileReferences Class-like references made outside any + * named class-like scope in this file */ public function storeClassNodes( string $file, @@ -292,6 +308,7 @@ public function storeClassNodes( array $classNodes, ?FileAnalysis $fileAnalysis = null, array $anonymousClassNodes = [], + array $fileReferences = [], ): void { $this->ensureCacheInitialised(); @@ -299,6 +316,7 @@ public function storeClassNodes( 'metadata' => $this->fileMetadata($file, $namespace), 'nodes' => array_map($this->classNodeToArray(...), $classNodes), 'anonymousClassNodes' => array_map($this->anonymousClassNodeToArray(...), $anonymousClassNodes), + 'fileReferences' => $fileReferences, ]; if ($fileAnalysis instanceof FileAnalysis) { @@ -382,6 +400,17 @@ className: $className, ); } + /** + * @param array $payload + * @return list|null + */ + private function fileReferencesFromPayload(array $payload): ?array + { + $fileReferences = $payload['fileReferences'] ?? []; + + return $this->isStringArray($fileReferences) ? array_values($fileReferences) : null; + } + /** * @return array */ diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index e2ab98f1..aeda5d1b 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -422,6 +422,78 @@ public function testYagniRulesIgnoreSelfReferences(): void $this->assertSame('App\UnusedInterface', $violations[0]->className); } + public function testYagniRulesDoNotFlagAbstractionsReferencedByProceduralCode(): void + { + $functions = 'makeTempProject([ + 'src/Contract.php' => ' ' ' $functions, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + // Type hints and ::class references in procedural code have no + // ClassNode of their own but still keep the abstractions alive. + $this->assertFalse($ruleViolationCollection->hasViolations()); + } + + public function testYagniRulesDoNotFlagAbstractionsReferencedByTopLevelAnonymousClassBody(): void + { + // Migration-style file: no named class at all; the reference lives in + // the anonymous class body, not in its extends/implements/use clauses. + $registration = 'makeTempProject([ + 'src/Contract.php' => ' $registration, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED)); + } + + public function testYagniRulesRecognizeProceduralReferencesOnCachedRun(): void + { + $functions = 'makeTempProject([ + 'src/Contract.php' => ' $functions, + ]); + $analysisResultCache = new AnalysisResultCache($basePath, new FileHashProvider(), 'cache'); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $coldViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + $warmViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + // Procedural references must survive the class-node cache round-trip. + $this->assertFalse($coldViolationCollection->hasViolations()); + $this->assertFalse($warmViolationCollection->hasViolations()); + } + public function testYagniRulesDoNotFlagAbstractionsUsedByAnonymousClass(): void { $factory = ' ' ' ' ' $consumer, + 'src/functions.php' => ' [], + 'fileAnalyses' => [], + 'anonymousClassNodes' => [], + 'fileReferences' => 'invalid', + 'error' => null, + ]; + + $dir = $this->makeTemporaryDirectory('structarmed-parallel-test'); + $file = $dir . '/Foo.php'; + file_put_contents($file, 'expectException(RuntimeException::class); + $this->expectExceptionMessage('Parallel analysis worker returned invalid file references.'); + + try { + $parallelClassNodeExtractor->extract([$file]); + } finally { + $GLOBALS['mock_file_get_contents_payload'] = null; + $GLOBALS['mock_tracked_tempnam_files'] = []; + } + } + + public function testExtractThrowsWhenFileReferencesEntryIsInvalid(): void + { + $GLOBALS['mock_file_get_contents_payload'] = [ + 'nodes' => [], + 'fileAnalyses' => [], + 'anonymousClassNodes' => [], + 'fileReferences' => ['Foo.php' => 'invalid'], + 'error' => null, + ]; + + $dir = $this->makeTemporaryDirectory('structarmed-parallel-test'); + $file = $dir . '/Foo.php'; + file_put_contents($file, 'expectException(RuntimeException::class); + $this->expectExceptionMessage('Parallel analysis worker returned invalid file references.'); + + try { + $parallelClassNodeExtractor->extract([$file]); + } finally { + $GLOBALS['mock_file_get_contents_payload'] = null; + $GLOBALS['mock_tracked_tempnam_files'] = []; + } + } } diff --git a/tests/Cache/AnalysisResultCacheTest.php b/tests/Cache/AnalysisResultCacheTest.php index 268e4dc8..221c08d6 100644 --- a/tests/Cache/AnalysisResultCacheTest.php +++ b/tests/Cache/AnalysisResultCacheTest.php @@ -620,6 +620,7 @@ traits: ['App\Helper'], $classNodes, null, $anonymousClassNodes, + ['App\ReferencedInFunction'], ); $loaded = $analysisResultCache->loadClassNodes($sourceFile, 'config'); @@ -627,6 +628,7 @@ traits: ['App\Helper'], $this->assertIsArray($loaded); $this->assertEquals($classNodes, $loaded['classNodes']); $this->assertEquals($anonymousClassNodes, $loaded['anonymousClassNodes']); + $this->assertSame(['App\ReferencedInFunction'], $loaded['fileReferences']); } finally { if (file_exists($sourceFile)) { unlink($sourceFile); @@ -660,6 +662,7 @@ public function testClassNodesLoadOldCachePayloadWithoutAnonymousClassNodes(): v $this->assertIsArray($loaded); $this->assertEquals($classNodes, $loaded['classNodes']); $this->assertSame([], $loaded['anonymousClassNodes']); + $this->assertSame([], $loaded['fileReferences']); } finally { if (file_exists($sourceFile)) { unlink($sourceFile); @@ -685,6 +688,33 @@ public static function corruptedAnonymousClassNodesProvider(): Iterator ]; } + public function testLoadClassNodesRejectsCorruptedFileReferencesPayload(): void + { + $cacheDirectory = $this->createTempDirectory(); + $sourceFile = $cacheDirectory . '/Foo.php'; + $analysisResultCache = new AnalysisResultCache(__DIR__, new FileHashProvider(), $cacheDirectory); + + file_put_contents($sourceFile, 'storeClassNodes($sourceFile, 'config', [$this->makeClassNode($sourceFile)]); + + $cacheFile = $this->firstJsonFile($cacheDirectory); + $payload = json_decode((string) file_get_contents($cacheFile), true); + $this->assertIsArray($payload); + $payload['fileReferences'] = ['App\Contract', 1]; + file_put_contents($cacheFile, json_encode($payload, JSON_THROW_ON_ERROR)); + + $this->assertNull($analysisResultCache->loadClassNodes($sourceFile, 'config')); + } finally { + if (file_exists($sourceFile)) { + unlink($sourceFile); + } + + $this->removeTempDirectory($cacheDirectory); + } + } + #[DataProvider('corruptedAnonymousClassNodesProvider')] public function testLoadClassNodesRejectsCorruptedAnonymousClassNodesPayload(mixed $corrupted): void { From 47133ac615d9d219a245481a102638cded2de888 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 03:14:19 +0700 Subject: [PATCH 07/19] update parallel file references --- src/Analyser/ClassCollector.php | 6 +++--- .../Parallel/ParallelClassNodeExtractor.php | 14 +++++++++++++- tests/Analyser/AnalyserTest.php | 4 ++-- .../Parallel/ParallelClassNodeExtractorTest.php | 16 ++++++++++++++-- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index b2ea48ae..22c4c199 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -150,9 +150,9 @@ public function getAnonymousClassNodes(): array } /** - * Class-like references made outside any named class-like scope, per file — - * procedural functions, top-level statements, and top-level anonymous - * class bodies. + * References to class-likes made outside any named class-like scope, per + * file — procedural functions, top-level statements, and top-level + * anonymous class bodies. * * @return array> */ diff --git a/src/Analyser/Parallel/ParallelClassNodeExtractor.php b/src/Analyser/Parallel/ParallelClassNodeExtractor.php index 15e14638..78083f24 100644 --- a/src/Analyser/Parallel/ParallelClassNodeExtractor.php +++ b/src/Analyser/Parallel/ParallelClassNodeExtractor.php @@ -261,7 +261,19 @@ public function extract( ); } - $fileReferences[$file] = $references; + $validReferences = []; + + foreach ($references as $reference) { + if (! is_string($reference)) { + throw new RuntimeException( + 'Parallel analysis worker returned invalid file references.' + ); + } + + $validReferences[] = $reference; + } + + $fileReferences[$file] = $validReferences; } } catch (RuntimeException $runtimeException) { $failure ??= $runtimeException->getMessage(); diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index aeda5d1b..df0ed92c 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -484,13 +484,13 @@ public function testYagniRulesRecognizeProceduralReferencesOnCachedRun(): void $architecture = Architecture::define() ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); - $coldViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) + $ruleViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) ->analyse($architecture, [], null, AnalyserOptions::sequential()); $warmViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) ->analyse($architecture, [], null, AnalyserOptions::sequential()); // Procedural references must survive the class-node cache round-trip. - $this->assertFalse($coldViolationCollection->hasViolations()); + $this->assertFalse($ruleViolationCollection->hasViolations()); $this->assertFalse($warmViolationCollection->hasViolations()); } diff --git a/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php b/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php index 0a2d0496..e8ed6de0 100644 --- a/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php +++ b/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php @@ -7,7 +7,9 @@ use Boundwize\StructArmed\Analyser\ClassNode; use Boundwize\StructArmed\Analyser\Parallel\ParallelClassNodeExtractor; use Boundwize\StructArmed\Tests\Support\TemporaryDirectoryCleanupTrait; +use Iterator; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use RuntimeException; @@ -479,13 +481,23 @@ public function testExtractThrowsWhenFileReferencesPayloadIsNotAnArray(): void } } - public function testExtractThrowsWhenFileReferencesEntryIsInvalid(): void + /** + * @return Iterator + */ + public static function invalidFileReferencesEntryProvider(): Iterator + { + yield 'entry not an array' => [['Foo.php' => 'invalid']]; + yield 'entry with non-string reference' => [['Foo.php' => [1]]]; + } + + #[DataProvider('invalidFileReferencesEntryProvider')] + public function testExtractThrowsWhenFileReferencesEntryIsInvalid(mixed $invalidFileReferences): void { $GLOBALS['mock_file_get_contents_payload'] = [ 'nodes' => [], 'fileAnalyses' => [], 'anonymousClassNodes' => [], - 'fileReferences' => ['Foo.php' => 'invalid'], + 'fileReferences' => $invalidFileReferences, 'error' => null, ]; From 0a74e2efdf859a6fb1ca79f695c7824bff51729e Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 03:20:12 +0700 Subject: [PATCH 08/19] add more tests --- tests/Analyser/AnalyserTest.php | 26 ++++++++++++++++++++++++++ tests/Analyser/ClassCollectorTest.php | 27 +++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index df0ed92c..585559c8 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -494,6 +494,32 @@ public function testYagniRulesRecognizeProceduralReferencesOnCachedRun(): void $this->assertFalse($warmViolationCollection->hasViolations()); } + public function testYagniRulesRecognizeProceduralReferencesOnCachedRunWithFileAnalysis(): void + { + $functions = 'makeTempProject([ + 'src/Contract.php' => ' $functions, + ]); + $analysisResultCache = new AnalysisResultCache($basePath, new FileHashProvider(), 'cache'); + + // A file-analysis rule makes the warm run load class nodes through the + // file-analysis cache path, which must also restore file references. + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])) + ->rule('psr1.php_tags', new Psr1PhpTagsRule(['src/'])); + + $ruleViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + $warmViolationCollection = (new Analyser($basePath, $analysisResultCache, 'config')) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + $this->assertFalse($ruleViolationCollection->hasViolations()); + $this->assertFalse($warmViolationCollection->hasViolations()); + } + public function testYagniRulesDoNotFlagAbstractionsUsedByAnonymousClass(): void { $factory = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Contract']], + $classCollector->getFileReferences() + ); + } + + public function testDoesNotCollectFileReferencesFromClassBodies(): void + { + $code = 'makeCollector($code); + + // References inside a named class-like land on its ClassNode + // dependencies, not in the file-level references. + $this->assertSame([], $classCollector->getFileReferences()); + } + public function testCollectsFinalClass(): void { $classNode = $this->collect(' Date: Wed, 19 Aug 2026 10:20:52 +0700 Subject: [PATCH 09/19] rename classes/interfaces --- docs/available-rules.md | 8 +- docs/presets.md | 2 +- src/Analyser/Analyser.php | 30 +++--- src/Analyser/ClassCollector.php | 25 +++++ src/Analyser/ClassNode.php | 12 +-- src/Preset/Presets/YagniPreset.php | 16 +-- ...le.php => MustBeUsedAbstractClassRule.php} | 6 +- ...ceRule.php => MustBeUsedInterfaceRule.php} | 11 ++- src/Rule/Rules/Class_/MustBeUsedTraitRule.php | 4 +- ...hp => UsedInterfaceAwareRuleInterface.php} | 2 +- src/Rule/UsedTraitAwareRuleInterface.php | 4 +- tests/Analyser/AnalyserTest.php | 98 ++++++++++++++++--- tests/Analyser/ClassCollectorTest.php | 25 +++++ tests/Analyser/ClassNodeTest.php | 10 +- tests/Preset/PresetTest.php | 12 +-- ...hp => MustBeUsedAbstractClassRuleTest.php} | 88 ++++++++--------- ...st.php => MustBeUsedInterfaceRuleTest.php} | 94 +++++++++--------- tests/Rule/Class_/MustBeUsedTraitRuleTest.php | 8 +- 18 files changed, 291 insertions(+), 164 deletions(-) rename src/Rule/Rules/Class_/{MustBeOverriddenAbstractClassRule.php => MustBeUsedAbstractClassRule.php} (92%) rename src/Rule/Rules/Class_/{MustBeImplementedInterfaceRule.php => MustBeUsedInterfaceRule.php} (82%) rename src/Rule/{ImplementedInterfaceAwareRuleInterface.php => UsedInterfaceAwareRuleInterface.php} (89%) rename tests/Rule/Class_/{MustBeOverriddenAbstractClassRuleTest.php => MustBeUsedAbstractClassRuleTest.php} (55%) rename tests/Rule/Class_/{MustBeImplementedInterfaceRuleTest.php => MustBeUsedInterfaceRuleTest.php} (63%) diff --git a/docs/available-rules.md b/docs/available-rules.md index 65259195..4f334f4b 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -81,10 +81,10 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | -| `MustBeImplementedInterfaceRule` | `new MustBeImplementedInterfaceRule(layer: 'Source')` | Interfaces are implemented by a scanned class (directly or through inheritance), extended by another scanned interface, or referenced as a dependency (type hint, `instanceof`, `::class`, ...). Supports `--fix` by removing the unused interface (and deleting its file when only boilerplate remains). | +| `MustBeUsedInterfaceRule` | `new MustBeUsedInterfaceRule(layer: 'Source')` | Interfaces are implemented by a scanned class (directly or through inheritance), extended by another scanned interface, or referenced as a dependency (type hint, `instanceof`, `::class`, a class-name string, ...). Supports `--fix` by removing the unused interface (and deleting its file when only boilerplate remains). | | `MustBeInterfaceRule` | `new MustBeInterfaceRule(layer: 'Contract', classNamePattern: '/Interface$/')` | Matching declarations in a layer are interfaces. | -| `MustBeOverriddenAbstractClassRule` | `new MustBeOverriddenAbstractClassRule(layer: 'Source')` | Abstract classes are extended by a scanned class or referenced as a dependency (type hint, `instanceof`, `::class`, static call, ...). Supports `--fix` by removing the unused abstract class (and deleting its file when only boilerplate remains). | -| `MustBeUsedTraitRule` | `new MustBeUsedTraitRule(layer: 'Source')` | Traits are used by a scanned class, trait, or enum, or referenced as a dependency (`::class`, static call, ...). Supports `--fix` by removing the unused trait (and deleting its file when only boilerplate remains). | +| `MustBeUsedAbstractClassRule` | `new MustBeUsedAbstractClassRule(layer: 'Source')` | Abstract classes are extended by a scanned class or referenced as a dependency (type hint, `instanceof`, `::class`, static call, a class-name string, ...). Supports `--fix` by removing the unused abstract class (and deleting its file when only boilerplate remains). | +| `MustBeUsedTraitRule` | `new MustBeUsedTraitRule(layer: 'Source')` | Traits are used by a scanned class, trait, or enum, or referenced as a dependency (`::class`, static call, a class-name string, ...). Supports `--fix` by removing the unused trait (and deleting its file when only boilerplate remains). | | `MustDeclareConstantVisibilityRule` | `new MustDeclareConstantVisibilityRule(layer: 'Source')` | Class constants declare `public`, `protected`, or `private`. Supports `--fix`. | | `MustDeclareMethodVisibilityRule` | `new MustDeclareMethodVisibilityRule(layer: 'Source')` | Methods declare `public`, `protected`, or `private`. Supports `--fix`. | | `MustDeclarePropertyVisibilityRule` | `new MustDeclarePropertyVisibilityRule(layer: 'Source')` | Properties declare `public`, `protected`, or `private`. Supports `--fix`. | @@ -94,7 +94,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. `classNamePattern` and `excludePattern` are regular expressions matched against the fully-qualified class name. -`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `MustBeFinalRule`, `MustBeImplementedInterfaceRule`, `MustBeOverriddenAbstractClassRule`, `MustBeUsedTraitRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` class modifier, remove unused interfaces, abstract classes, and traits (deleting their file when only `declare`/`namespace`/`use` boilerplate remains), and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. +`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `MustBeFinalRule`, `MustBeUsedInterfaceRule`, `MustBeUsedAbstractClassRule`, `MustBeUsedTraitRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` class modifier, remove unused interfaces, abstract classes, and traits (deleting their file when only `declare`/`namespace`/`use` boilerplate remains), and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. ## Layer Rules diff --git a/docs/presets.md b/docs/presets.md index 7104ad2f..1533a816 100644 --- a/docs/presets.md +++ b/docs/presets.md @@ -25,7 +25,7 @@ StructArmed ships with presets for common PHP standards and architecture styles. | `Preset::PSR4()` | Verifies configured source paths exist in composer.json `autoload` or `autoload-dev` PSR-4 mappings | | `Preset::DDD()` | Layer isolation, entity/VO/repository/event/service conventions | | `Preset::MVC()` | Layer isolation, thin controllers, model/view/service rules | -| `Preset::YAGNI()` | Speculative-abstraction cleanup: interfaces must be implemented by a class or extended by another interface, abstract classes must be extended, traits must be used within the scanned paths. All three rules support `--fix` by removing the unused declaration | +| `Preset::YAGNI()` | Speculative-abstraction cleanup: interfaces must be implemented by a class or extended by another interface, abstract classes must be extended, traits must be used within the scanned paths — a dependency reference (type hint, `instanceof`, `::class`, static call, a class-name string, ...) also counts as usage. All three rules support `--fix` by removing the unused declaration | ## Initialize Presets diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 002f0f12..ca168c71 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -17,7 +17,6 @@ use Boundwize\StructArmed\Rule\ExtendedClassAwareRuleInterface; use Boundwize\StructArmed\Rule\FileAnalysisRuleInterface; use Boundwize\StructArmed\Rule\FixableInterface; -use Boundwize\StructArmed\Rule\ImplementedInterfaceAwareRuleInterface; use Boundwize\StructArmed\Rule\LayerAwareRuleInterface; use Boundwize\StructArmed\Rule\MultipleProjectRuleViolationInterface; use Boundwize\StructArmed\Rule\MultipleRuleViolationInterface; @@ -25,6 +24,7 @@ use Boundwize\StructArmed\Rule\RuleInterface; use Boundwize\StructArmed\Rule\RuleViolation; use Boundwize\StructArmed\Rule\RuleViolationCollection; +use Boundwize\StructArmed\Rule\UsedInterfaceAwareRuleInterface; use Boundwize\StructArmed\Rule\UsedTraitAwareRuleInterface; use Boundwize\StructArmed\Util\Path; @@ -80,13 +80,13 @@ public function analyse( $ruleSkipPaths = $architecture->getRuleSkipPaths(); $skippedRuleKeys = $this->skippedRuleKeyMap($architecture->getSkippedRuleKeys()); - $projectRuleViolations = []; - $fileAnalysisRules = []; - $classRules = []; - $layerAwareRules = []; - $hasExtendedClassAwareRule = false; - $hasImplementedInterfaceAwareRule = false; - $hasUsedTraitAwareRule = false; + $projectRuleViolations = []; + $fileAnalysisRules = []; + $classRules = []; + $layerAwareRules = []; + $hasExtendedClassAwareRule = false; + $hasUsedInterfaceAwareRule = false; + $hasUsedTraitAwareRule = false; foreach ($rules as $key => $rule) { if (array_key_exists($key, $skippedRuleKeys)) { @@ -105,8 +105,8 @@ public function analyse( $hasExtendedClassAwareRule = true; } - if ($rule instanceof ImplementedInterfaceAwareRuleInterface) { - $hasImplementedInterfaceAwareRule = true; + if ($rule instanceof UsedInterfaceAwareRuleInterface) { + $hasUsedInterfaceAwareRule = true; } if ($rule instanceof UsedTraitAwareRuleInterface) { @@ -164,12 +164,12 @@ public function analyse( $this->markExtendedClasses($classNodes, $extractionResult); } - if ($hasImplementedInterfaceAwareRule) { + if ($hasUsedInterfaceAwareRule) { $this->markImplementedInterfaces($classNodes, $extractionResult); } - if ($hasExtendedClassAwareRule || $hasImplementedInterfaceAwareRule || $hasUsedTraitAwareRule) { - $this->markUsedClassLikes($classNodes, $extractionResult); + if ($hasExtendedClassAwareRule || $hasUsedInterfaceAwareRule || $hasUsedTraitAwareRule) { + $this->markReferencedClassLikes($classNodes, $extractionResult); } if ($withFileAnalysis) { @@ -753,7 +753,7 @@ private function markImplementedInterfaces(array $classNodes, ExtractionResult $ * * @param list $classNodes */ - private function markUsedClassLikes(array $classNodes, ExtractionResult $extractionResult): void + private function markReferencedClassLikes(array $classNodes, ExtractionResult $extractionResult): void { $used = []; @@ -792,7 +792,7 @@ private function markUsedClassLikes(array $classNodes, ExtractionResult $extract foreach ($classNodes as $classNode) { if (isset($used[strtolower($classNode->className)])) { - $classNode->setUsed(true); + $classNode->setReferenced(true); } } } diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index 22c4c199..e831be4d 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -26,6 +26,7 @@ use PhpParser\Node\Name; use PhpParser\Node\Name\FullyQualified; use PhpParser\Node\Param; +use PhpParser\Node\Scalar\String_; use PhpParser\Node\Stmt\Case_; use PhpParser\Node\Stmt\Catch_; use PhpParser\Node\Stmt\Class_; @@ -57,6 +58,7 @@ use function count; use function in_array; use function is_string; +use function preg_match; use function spl_object_id; use function strtolower; @@ -80,6 +82,14 @@ final class ClassCollector extends NodeVisitorAbstract 'null' => true, ]; + /** + * A string value shaped like a (possibly namespaced) class name, e.g. + * 'App\Contract' or 'stdClass'. Such values can reach `new $class` or + * `instanceof $class` at runtime, so they count as references. + */ + private const CLASS_LIKE_STRING_PATTERN = + '/^[A-Za-z_\x80-\xff][A-Za-z0-9_\x80-\xff]*+(?:\\\\[A-Za-z_\x80-\xff][A-Za-z0-9_\x80-\xff]*+)*+$/'; + /** @var list */ private array $nodes = []; @@ -318,6 +328,21 @@ private function finishMethodAnalysis(ClassMethod $classMethod): void private function collectNodeAnalysis(Node $node): void { + // A class-name-shaped string literal may feed `new $class`, + // `$obj instanceof $class`, class_exists(), container ids, and so on. + // Whether it appears inside a class-like or in procedural code, treat + // it as a file-level reference so the named class-like stays alive. + if ($node instanceof String_) { + if ( + ! isset(self::KEYWORD_CONSTANTS[strtolower($node->value)]) + && preg_match(self::CLASS_LIKE_STRING_PATTERN, $node->value) === 1 + ) { + $this->currentFileReferences[] = $node->value; + } + + return; + } + if ($this->activeClassLikeAnalyses === []) { // Outside any named class-like scope — procedural functions, // top-level statements, top-level anonymous class bodies — a diff --git a/src/Analyser/ClassNode.php b/src/Analyser/ClassNode.php index a03a62aa..4989ca20 100644 --- a/src/Analyser/ClassNode.php +++ b/src/Analyser/ClassNode.php @@ -61,7 +61,7 @@ public function __construct( public array $parentInterfaces = [], public bool $isExtended = false, public bool $isImplemented = false, - public bool $isUsed = false, + public bool $isReferenced = false, ) { $this->layers = $layers ?: array_filter([$this->layer]); } @@ -88,7 +88,7 @@ public function setExtended(bool $isExtended): void /** * Whether another scanned class implements this interface (directly or * through inheritance) or another scanned interface extends it. Computed by - * the analyser for rules implementing ImplementedInterfaceAwareRuleInterface; + * the analyser for rules implementing UsedInterfaceAwareRuleInterface; * false otherwise. */ public function setImplemented(bool $isImplemented): void @@ -97,14 +97,14 @@ public function setImplemented(bool $isImplemented): void } /** - * Whether another scanned class-like uses this class-like — as a trait, or - * by referencing it as a dependency (type hint, instanceof, ::class, + * Whether another scanned class-like references this class-like — as a + * trait it uses, or as a dependency (type hint, instanceof, ::class, * static call, ...). Computed by the analyser when a usage-aware rule is * active; false otherwise. */ - public function setUsed(bool $isUsed): void + public function setReferenced(bool $isReferenced): void { - $this->isUsed = $isUsed; + $this->isReferenced = $isReferenced; } public function shortName(): string diff --git a/src/Preset/Presets/YagniPreset.php b/src/Preset/Presets/YagniPreset.php index 0aea4ede..70865257 100644 --- a/src/Preset/Presets/YagniPreset.php +++ b/src/Preset/Presets/YagniPreset.php @@ -6,8 +6,8 @@ use Boundwize\StructArmed\Architecture; use Boundwize\StructArmed\Preset\PresetInterface; -use Boundwize\StructArmed\Rule\Rules\Class_\MustBeImplementedInterfaceRule; -use Boundwize\StructArmed\Rule\Rules\Class_\MustBeOverriddenAbstractClassRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedInterfaceRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedTraitRule; /** @@ -24,9 +24,9 @@ { use ResolvesSourceLayerNameTrait; - public const INTERFACE_MUST_BE_IMPLEMENTED = 'yagni.interface.must_be_implemented'; + public const INTERFACE_MUST_BE_USED = 'yagni.interface.must_be_used'; - public const ABSTRACT_CLASS_MUST_BE_OVERRIDDEN = 'yagni.abstract_class.must_be_overridden'; + public const ABSTRACT_CLASS_MUST_BE_USED = 'yagni.abstract_class.must_be_used'; public const TRAIT_MUST_BE_USED = 'yagni.trait.must_be_used'; @@ -44,12 +44,12 @@ public function apply(Architecture $architecture): void $architecture->layer($layerName, $this->sourcePaths ?? []); $architecture->rule( - self::INTERFACE_MUST_BE_IMPLEMENTED, - new MustBeImplementedInterfaceRule($layerName) + self::INTERFACE_MUST_BE_USED, + new MustBeUsedInterfaceRule($layerName) ); $architecture->rule( - self::ABSTRACT_CLASS_MUST_BE_OVERRIDDEN, - new MustBeOverriddenAbstractClassRule($layerName) + self::ABSTRACT_CLASS_MUST_BE_USED, + new MustBeUsedAbstractClassRule($layerName) ); $architecture->rule( self::TRAIT_MUST_BE_USED, diff --git a/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php b/src/Rule/Rules/Class_/MustBeUsedAbstractClassRule.php similarity index 92% rename from src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php rename to src/Rule/Rules/Class_/MustBeUsedAbstractClassRule.php index 16e8c672..2f9262b9 100644 --- a/src/Rule/Rules/Class_/MustBeOverriddenAbstractClassRule.php +++ b/src/Rule/Rules/Class_/MustBeUsedAbstractClassRule.php @@ -12,7 +12,7 @@ use function sprintf; -final readonly class MustBeOverriddenAbstractClassRule extends AbstractPhpParserFixableRule implements +final readonly class MustBeUsedAbstractClassRule extends AbstractPhpParserFixableRule implements ExtendedClassAwareRuleInterface { public function __construct( @@ -46,13 +46,13 @@ public function evaluate(ClassNode $classNode): ?RuleViolation // A dependency reference (instanceof, type hint, ::class, static // call, ...) means removing the class would break the referencing code. - if ($classNode->isUsed) { + if ($classNode->isReferenced) { return null; } return new RuleViolation( message: sprintf( - 'Abstract class [%s] must be extended by a class', + 'Abstract class [%s] must be extended by a class or referenced as a dependency', $classNode->className ), file: $classNode->file, diff --git a/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php b/src/Rule/Rules/Class_/MustBeUsedInterfaceRule.php similarity index 82% rename from src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php rename to src/Rule/Rules/Class_/MustBeUsedInterfaceRule.php index ad0831b0..4cd8d35c 100644 --- a/src/Rule/Rules/Class_/MustBeImplementedInterfaceRule.php +++ b/src/Rule/Rules/Class_/MustBeUsedInterfaceRule.php @@ -7,13 +7,13 @@ use Boundwize\StructArmed\Analyser\ClassNode; use Boundwize\StructArmed\Rule\Fixer\PhpParser\AbstractPhpParserFixableRule; use Boundwize\StructArmed\Rule\Fixer\PhpParser\ClassLike\RemoveClassLikeVisitor; -use Boundwize\StructArmed\Rule\ImplementedInterfaceAwareRuleInterface; use Boundwize\StructArmed\Rule\RuleViolation; +use Boundwize\StructArmed\Rule\UsedInterfaceAwareRuleInterface; use function sprintf; -final readonly class MustBeImplementedInterfaceRule extends AbstractPhpParserFixableRule implements - ImplementedInterfaceAwareRuleInterface +final readonly class MustBeUsedInterfaceRule extends AbstractPhpParserFixableRule implements + UsedInterfaceAwareRuleInterface { public function __construct( private string $layer, @@ -46,13 +46,14 @@ public function evaluate(ClassNode $classNode): ?RuleViolation // A dependency reference (instanceof, type hint, ::class, ...) means // removing the interface would break the referencing code. - if ($classNode->isUsed) { + if ($classNode->isReferenced) { return null; } return new RuleViolation( message: sprintf( - 'Interface [%s] must be implemented by a class or extended by another interface', + 'Interface [%s] must be implemented by a class, extended by another interface,' + . ' or referenced as a dependency', $classNode->className ), file: $classNode->file, diff --git a/src/Rule/Rules/Class_/MustBeUsedTraitRule.php b/src/Rule/Rules/Class_/MustBeUsedTraitRule.php index eca51606..8c038f49 100644 --- a/src/Rule/Rules/Class_/MustBeUsedTraitRule.php +++ b/src/Rule/Rules/Class_/MustBeUsedTraitRule.php @@ -40,13 +40,13 @@ public function appliesTo(ClassNode $classNode): bool public function evaluate(ClassNode $classNode): ?RuleViolation { - if ($classNode->isUsed) { + if ($classNode->isReferenced) { return null; } return new RuleViolation( message: sprintf( - 'Trait [%s] must be used by a class, trait, or enum', + 'Trait [%s] must be used by a class, trait, or enum, or referenced as a dependency', $classNode->className ), file: $classNode->file, diff --git a/src/Rule/ImplementedInterfaceAwareRuleInterface.php b/src/Rule/UsedInterfaceAwareRuleInterface.php similarity index 89% rename from src/Rule/ImplementedInterfaceAwareRuleInterface.php rename to src/Rule/UsedInterfaceAwareRuleInterface.php index 59739bfd..ccf05bfb 100644 --- a/src/Rule/ImplementedInterfaceAwareRuleInterface.php +++ b/src/Rule/UsedInterfaceAwareRuleInterface.php @@ -15,6 +15,6 @@ * implemented solely by a consumer outside the scan is reported as if not * implemented. */ -interface ImplementedInterfaceAwareRuleInterface extends RuleInterface +interface UsedInterfaceAwareRuleInterface extends RuleInterface { } diff --git a/src/Rule/UsedTraitAwareRuleInterface.php b/src/Rule/UsedTraitAwareRuleInterface.php index 40e60bbc..eb5284e9 100644 --- a/src/Rule/UsedTraitAwareRuleInterface.php +++ b/src/Rule/UsedTraitAwareRuleInterface.php @@ -7,8 +7,8 @@ /** * Marker for rules whose evaluation depends on whether a trait is used by * another scanned class-like (class, trait, or enum). When at least one active - * rule implements this marker, the analyser flags each ClassNode's $isUsed - * before rules are evaluated, so implementers can read $classNode->isUsed. + * rule implements this marker, the analyser flags each ClassNode's $isReferenced + * before rules are evaluated, so implementers can read $classNode->isReferenced. * * Trade-off: only usage within the scanned paths is known. A trait used solely * by a consumer outside the scan is reported as if not used. diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index 585559c8..92d4edd8 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -336,8 +336,8 @@ public function testYagniPresetReportsOnlyUnusedAbstractions(): void $ruleViolationCollection = (new Analyser($basePath)) ->analyse($architecture, [], null, AnalyserOptions::sequential()); - $interfaceViolations = $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED); - $abstractViolations = $ruleViolationCollection->forRule(YagniPreset::ABSTRACT_CLASS_MUST_BE_OVERRIDDEN); + $interfaceViolations = $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_USED); + $abstractViolations = $ruleViolationCollection->forRule(YagniPreset::ABSTRACT_CLASS_MUST_BE_USED); $traitViolations = $ruleViolationCollection->forRule(YagniPreset::TRAIT_MUST_BE_USED); // BaseInterface is extended by ChildInterface, so only UnusedInterface @@ -357,7 +357,7 @@ public function testYagniPresetReportsOnlyUnusedAbstractions(): void $this->assertSame('App\UnusedTrait', $traitViolations[0]->className); } - public function testMustBeImplementedInterfaceRuleRecognizesTransitiveImplementation(): void + public function testMustBeUsedInterfaceRuleRecognizesTransitiveImplementation(): void { $basePath = $this->makeTempProject([ 'src/BaseInterface.php' => 'assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED)); + $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_USED)); + } + + public function testMustBeUsedTraitRuleRecognizesTraitUsedByAnotherTrait(): void + { + $basePath = $this->makeTempProject([ + 'src/InnerTrait.php' => ' ' 'withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::TRAIT_MUST_BE_USED); + + // InnerTrait is used by OuterTrait, which is used by Consumer. + $this->assertCount(0, $violations); + } + + public function testMustBeUsedTraitRuleRecognizesTraitUsedByEnum(): void + { + $basePath = $this->makeTempProject([ + 'src/Helper.php' => ' 'withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::TRAIT_MUST_BE_USED); + + $this->assertCount(0, $violations); } public function testYagniRulesDoNotFlagAbstractionsReferencedAsDependencies(): void @@ -408,18 +444,58 @@ public function testYagniRulesIgnoreSelfReferences(): void $basePath = $this->makeTempProject([ 'src/UnusedInterface.php' => ' ' 'withPreset(Preset::YAGNI(sourcePaths: ['src/'])); - $violations = (new Analyser($basePath)) - ->analyse($architecture, [], null, AnalyserOptions::sequential()) - ->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED); + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + $interfaceViolations = $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_USED); + $abstractViolations = $ruleViolationCollection->forRule(YagniPreset::ABSTRACT_CLASS_MUST_BE_USED); + $traitViolations = $ruleViolationCollection->forRule(YagniPreset::TRAIT_MUST_BE_USED); // A class-like referencing itself cannot keep itself alive. - $this->assertCount(1, $violations); - $this->assertSame('App\UnusedInterface', $violations[0]->className); + $this->assertCount(1, $interfaceViolations); + $this->assertSame('App\UnusedInterface', $interfaceViolations[0]->className); + $this->assertCount(1, $abstractViolations); + $this->assertSame('App\UnusedBase', $abstractViolations[0]->className); + $this->assertCount(1, $traitViolations); + $this->assertSame('App\UnusedTrait', $traitViolations[0]->className); + } + + public function testYagniRulesDoNotFlagAbstractionsReferencedByClassNameString(): void + { + $checker = 'makeTempProject([ + 'src/Contract.php' => ' ' ' $checker, + 'src/bootstrap.php' => 'withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()); + + // A class-name string value can reach `new $class` or `instanceof + // $class` at runtime, so it counts as a reference — in class bodies + // and procedural code alike. + $this->assertFalse($ruleViolationCollection->hasViolations()); } public function testYagniRulesDoNotFlagAbstractionsReferencedByProceduralCode(): void @@ -467,7 +543,7 @@ public function testYagniRulesDoNotFlagAbstractionsReferencedByTopLevelAnonymous $ruleViolationCollection = (new Analyser($basePath)) ->analyse($architecture, [], null, AnalyserOptions::sequential()); - $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED)); + $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_USED)); } public function testYagniRulesRecognizeProceduralReferencesOnCachedRun(): void @@ -539,7 +615,7 @@ public function testYagniRulesDoNotFlagAbstractionsUsedByAnonymousClass(): void $ruleViolationCollection = (new Analyser($basePath)) ->analyse($architecture, [], null, AnalyserOptions::sequential()); - $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED)); + $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::INTERFACE_MUST_BE_USED)); $this->assertCount(0, $ruleViolationCollection->forRule(YagniPreset::TRAIT_MUST_BE_USED)); } diff --git a/tests/Analyser/ClassCollectorTest.php b/tests/Analyser/ClassCollectorTest.php index c0105c8c..004cf1b6 100644 --- a/tests/Analyser/ClassCollectorTest.php +++ b/tests/Analyser/ClassCollectorTest.php @@ -89,6 +89,31 @@ public function testDoesNotCollectFileReferencesFromClassBodies(): void $this->assertSame([], $classCollector->getFileReferences()); } + public function testCollectsClassNameShapedStringValuesAsFileReferences(): void + { + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Contract']], + $classCollector->getFileReferences() + ); + } + + public function testDoesNotCollectNonClassNameShapedStringValues(): void + { + $code = 'makeCollector($code); + + $this->assertSame([], $classCollector->getFileReferences()); + } + public function testCollectsFinalClass(): void { $classNode = $this->collect('assertFalse($classNode->isUsed); + $this->assertFalse($classNode->isReferenced); - $classNode->setUsed(true); - $this->assertTrue($classNode->isUsed); + $classNode->setReferenced(true); + $this->assertTrue($classNode->isReferenced); - $classNode->setUsed(false); - $this->assertFalse($classNode->isUsed); + $classNode->setReferenced(false); + $this->assertFalse($classNode->isReferenced); } public function testDependsOnMatchesExistingClassesExactly(): void diff --git a/tests/Preset/PresetTest.php b/tests/Preset/PresetTest.php index e2693757..8f189618 100644 --- a/tests/Preset/PresetTest.php +++ b/tests/Preset/PresetTest.php @@ -14,8 +14,8 @@ use Boundwize\StructArmed\Preset\Presets\Psr4Preset; use Boundwize\StructArmed\Preset\Presets\ResolvesSourceLayerNameTrait; use Boundwize\StructArmed\Preset\Presets\YagniPreset; -use Boundwize\StructArmed\Rule\Rules\Class_\MustBeImplementedInterfaceRule; -use Boundwize\StructArmed\Rule\Rules\Class_\MustBeOverriddenAbstractClassRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedInterfaceRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedTraitRule; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; @@ -43,12 +43,12 @@ public function testYagniPresetRegistersSourceLayerAndRules(): void $rules = $architecture->getRules(); $this->assertInstanceOf( - MustBeImplementedInterfaceRule::class, - $rules[YagniPreset::INTERFACE_MUST_BE_IMPLEMENTED] ?? null + MustBeUsedInterfaceRule::class, + $rules[YagniPreset::INTERFACE_MUST_BE_USED] ?? null ); $this->assertInstanceOf( - MustBeOverriddenAbstractClassRule::class, - $rules[YagniPreset::ABSTRACT_CLASS_MUST_BE_OVERRIDDEN] ?? null + MustBeUsedAbstractClassRule::class, + $rules[YagniPreset::ABSTRACT_CLASS_MUST_BE_USED] ?? null ); $this->assertInstanceOf( MustBeUsedTraitRule::class, diff --git a/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php b/tests/Rule/Class_/MustBeUsedAbstractClassRuleTest.php similarity index 55% rename from tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php rename to tests/Rule/Class_/MustBeUsedAbstractClassRuleTest.php index b928f8e3..1aa7e833 100644 --- a/tests/Rule/Class_/MustBeOverriddenAbstractClassRuleTest.php +++ b/tests/Rule/Class_/MustBeUsedAbstractClassRuleTest.php @@ -8,7 +8,7 @@ use Boundwize\StructArmed\Rule\ExtendedClassAwareRuleInterface; use Boundwize\StructArmed\Rule\FixableInterface; use Boundwize\StructArmed\Rule\Fixer\PhpParser\ClassLike\RemoveClassLikeVisitor; -use Boundwize\StructArmed\Rule\Rules\Class_\MustBeOverriddenAbstractClassRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; use Boundwize\StructArmed\Rule\RuleViolation; use Boundwize\StructArmed\Tests\Support\TemporaryDirectoryCleanupTrait; use PHPUnit\Framework\Attributes\CoversClass; @@ -17,9 +17,9 @@ use function file_put_contents; -#[CoversClass(MustBeOverriddenAbstractClassRule::class)] +#[CoversClass(MustBeUsedAbstractClassRule::class)] #[CoversClass(RemoveClassLikeVisitor::class)] -final class MustBeOverriddenAbstractClassRuleTest extends TestCase +final class MustBeUsedAbstractClassRuleTest extends TestCase { use TemporaryDirectoryCleanupTrait; @@ -31,7 +31,7 @@ private function makeNode( bool $isTrait = false, bool $isEnum = false, bool $isExtended = false, - bool $isUsed = false, + bool $isReferenced = false, ): ClassNode { return new ClassNode( className: $className, @@ -46,38 +46,38 @@ className: $className, isTrait: $isTrait, isEnum: $isEnum, isExtended: $isExtended, - isUsed: $isUsed, + isReferenced: $isReferenced, ); } public function testPassesWhenAbstractClassIsExtended(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(isExtended: true); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isExtended: true); $this->assertNotInstanceOf( RuleViolation::class, - $mustBeOverriddenAbstractClassRule->evaluate($classNode) + $mustBeUsedAbstractClassRule->evaluate($classNode) ); } public function testPassesWhenAbstractClassIsReferencedAsDependency(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(isUsed: true); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isReferenced: true); $this->assertNotInstanceOf( RuleViolation::class, - $mustBeOverriddenAbstractClassRule->evaluate($classNode) + $mustBeUsedAbstractClassRule->evaluate($classNode) ); } public function testViolatesWhenAbstractClassIsNotExtended(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(isExtended: false); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isExtended: false); - $violation = $mustBeOverriddenAbstractClassRule->evaluate($classNode); + $violation = $mustBeUsedAbstractClassRule->evaluate($classNode); $this->assertInstanceOf(RuleViolation::class, $violation); $this->assertStringContainsString('must be extended', $violation->message); @@ -87,24 +87,24 @@ public function testIsExtendedClassAware(): void { $this->assertInstanceOf( ExtendedClassAwareRuleInterface::class, - new MustBeOverriddenAbstractClassRule(layer: 'Domain') + new MustBeUsedAbstractClassRule(layer: 'Domain') ); } public function testIsFixable(): void { - $this->assertInstanceOf(FixableInterface::class, new MustBeOverriddenAbstractClassRule(layer: 'Domain')); + $this->assertInstanceOf(FixableInterface::class, new MustBeUsedAbstractClassRule(layer: 'Domain')); } public function testCreatesRemoveClassLikeFixerVisitor(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $reflectionMethod = new ReflectionMethod( - $mustBeOverriddenAbstractClassRule, + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $reflectionMethod = new ReflectionMethod( + $mustBeUsedAbstractClassRule, 'createFixerVisitor' ); - $removeClassLikeVisitor = $reflectionMethod->invoke( - $mustBeOverriddenAbstractClassRule, + $removeClassLikeVisitor = $reflectionMethod->invoke( + $mustBeUsedAbstractClassRule, new RuleViolation( message: 'Abstract class [App\\AbstractHandler] must be extended by a class', file: '/src/AbstractHandler.php', @@ -119,64 +119,64 @@ className: 'App\\AbstractHandler', public function testDoesNotApplyToWrongLayer(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(layer: 'Infrastructure'); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(layer: 'Infrastructure'); - $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedAbstractClassRule->appliesTo($classNode)); } public function testDoesNotApplyToConcreteClasses(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(isAbstract: false); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isAbstract: false); - $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedAbstractClassRule->appliesTo($classNode)); } public function testDoesNotApplyToInterfaces(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(isInterface: true); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isInterface: true); - $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedAbstractClassRule->appliesTo($classNode)); } public function testDoesNotApplyToTraits(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(isTrait: true); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(isTrait: true); - $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedAbstractClassRule->appliesTo($classNode)); } public function testAppliesToLayerWhenNoPatternConfigured(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule(layer: 'Domain'); - $classNode = $this->makeNode(); + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule(layer: 'Domain'); + $classNode = $this->makeNode(); - $this->assertTrue($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + $this->assertTrue($mustBeUsedAbstractClassRule->appliesTo($classNode)); } public function testAppliesToMatchingPattern(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule( + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule( layer: 'Domain', classNamePattern: '/^App\\\\Domain\\\\Abstract/' ); - $classNode = $this->makeNode(); + $classNode = $this->makeNode(); - $this->assertTrue($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + $this->assertTrue($mustBeUsedAbstractClassRule->appliesTo($classNode)); } public function testDoesNotApplyToNonMatchingPattern(): void { - $mustBeOverriddenAbstractClassRule = new MustBeOverriddenAbstractClassRule( + $mustBeUsedAbstractClassRule = new MustBeUsedAbstractClassRule( layer: 'Domain', classNamePattern: '/Base$/' ); - $classNode = $this->makeNode(); + $classNode = $this->makeNode(); - $this->assertFalse($mustBeOverriddenAbstractClassRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedAbstractClassRule->appliesTo($classNode)); } public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void @@ -189,9 +189,9 @@ public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void "assertTrue($mustBeOverriddenAbstractClassRule->fix(new RuleViolation( + $this->assertTrue($mustBeUsedAbstractClassRule->fix(new RuleViolation( message: 'Abstract class [App\\AbstractHandler] must be extended by a class', file: $file, line: 7, diff --git a/tests/Rule/Class_/MustBeImplementedInterfaceRuleTest.php b/tests/Rule/Class_/MustBeUsedInterfaceRuleTest.php similarity index 63% rename from tests/Rule/Class_/MustBeImplementedInterfaceRuleTest.php rename to tests/Rule/Class_/MustBeUsedInterfaceRuleTest.php index 2063c885..471408d1 100644 --- a/tests/Rule/Class_/MustBeImplementedInterfaceRuleTest.php +++ b/tests/Rule/Class_/MustBeUsedInterfaceRuleTest.php @@ -9,9 +9,9 @@ use Boundwize\StructArmed\Rule\Fixer\PhpParser\AbstractPhpParserFixableRule; use Boundwize\StructArmed\Rule\Fixer\PhpParser\ClassLike\RemoveClassLikeVisitor; use Boundwize\StructArmed\Rule\Fixer\PhpParser\PhpParserFixerProcessor; -use Boundwize\StructArmed\Rule\ImplementedInterfaceAwareRuleInterface; -use Boundwize\StructArmed\Rule\Rules\Class_\MustBeImplementedInterfaceRule; +use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedInterfaceRule; use Boundwize\StructArmed\Rule\RuleViolation; +use Boundwize\StructArmed\Rule\UsedInterfaceAwareRuleInterface; use Boundwize\StructArmed\Tests\Support\TemporaryDirectoryCleanupTrait; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; @@ -20,11 +20,11 @@ use function file_get_contents; use function file_put_contents; -#[CoversClass(MustBeImplementedInterfaceRule::class)] +#[CoversClass(MustBeUsedInterfaceRule::class)] #[CoversClass(RemoveClassLikeVisitor::class)] #[CoversClass(AbstractPhpParserFixableRule::class)] #[CoversClass(PhpParserFixerProcessor::class)] -final class MustBeImplementedInterfaceRuleTest extends TestCase +final class MustBeUsedInterfaceRuleTest extends TestCase { use TemporaryDirectoryCleanupTrait; @@ -35,7 +35,7 @@ private function makeNode( bool $isTrait = false, bool $isEnum = false, bool $isImplemented = false, - bool $isUsed = false, + bool $isReferenced = false, ): ClassNode { return new ClassNode( className: $className, @@ -50,62 +50,62 @@ className: $className, isTrait: $isTrait, isEnum: $isEnum, isImplemented: $isImplemented, - isUsed: $isUsed, + isReferenced: $isReferenced, ); } public function testPassesWhenInterfaceIsImplemented(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $classNode = $this->makeNode(isImplemented: true); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isImplemented: true); $this->assertNotInstanceOf( RuleViolation::class, - $mustBeImplementedInterfaceRule->evaluate($classNode) + $mustBeUsedInterfaceRule->evaluate($classNode) ); } public function testPassesWhenInterfaceIsReferencedAsDependency(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $classNode = $this->makeNode(isUsed: true); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isReferenced: true); $this->assertNotInstanceOf( RuleViolation::class, - $mustBeImplementedInterfaceRule->evaluate($classNode) + $mustBeUsedInterfaceRule->evaluate($classNode) ); } public function testViolatesWhenInterfaceIsNotImplemented(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $classNode = $this->makeNode(isImplemented: false); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isImplemented: false); - $violation = $mustBeImplementedInterfaceRule->evaluate($classNode); + $violation = $mustBeUsedInterfaceRule->evaluate($classNode); $this->assertInstanceOf(RuleViolation::class, $violation); $this->assertStringContainsString('must be implemented', $violation->message); } - public function testIsImplementedInterfaceAware(): void + public function testIsUsedInterfaceAware(): void { $this->assertInstanceOf( - ImplementedInterfaceAwareRuleInterface::class, - new MustBeImplementedInterfaceRule(layer: 'Domain') + UsedInterfaceAwareRuleInterface::class, + new MustBeUsedInterfaceRule(layer: 'Domain') ); } public function testIsFixable(): void { - $this->assertInstanceOf(FixableInterface::class, new MustBeImplementedInterfaceRule(layer: 'Domain')); + $this->assertInstanceOf(FixableInterface::class, new MustBeUsedInterfaceRule(layer: 'Domain')); } public function testCreatesRemoveClassLikeFixerVisitor(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $reflectionMethod = new ReflectionMethod($mustBeImplementedInterfaceRule, 'createFixerVisitor'); - $removeClassLikeVisitor = $reflectionMethod->invoke( - $mustBeImplementedInterfaceRule, + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $reflectionMethod = new ReflectionMethod($mustBeUsedInterfaceRule, 'createFixerVisitor'); + $removeClassLikeVisitor = $reflectionMethod->invoke( + $mustBeUsedInterfaceRule, new RuleViolation( message: 'Interface [App\\Unused] must be implemented by a class or extended by another interface', file: '/src/Unused.php', @@ -120,56 +120,56 @@ className: 'App\\Unused', public function testDoesNotApplyToWrongLayer(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $classNode = $this->makeNode(layer: 'Infrastructure'); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(layer: 'Infrastructure'); - $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedInterfaceRule->appliesTo($classNode)); } public function testDoesNotApplyToClasses(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $classNode = $this->makeNode(isInterface: false); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isInterface: false); - $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedInterfaceRule->appliesTo($classNode)); } public function testDoesNotApplyToTraits(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $classNode = $this->makeNode(isInterface: false, isTrait: true); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(isInterface: false, isTrait: true); - $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedInterfaceRule->appliesTo($classNode)); } public function testAppliesToLayerWhenNoPatternConfigured(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); - $classNode = $this->makeNode(); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); + $classNode = $this->makeNode(); - $this->assertTrue($mustBeImplementedInterfaceRule->appliesTo($classNode)); + $this->assertTrue($mustBeUsedInterfaceRule->appliesTo($classNode)); } public function testAppliesToMatchingPattern(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule( + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule( layer: 'Domain', classNamePattern: '/Interface$/' ); - $classNode = $this->makeNode(className: 'App\\Domain\\OrderRepositoryInterface'); + $classNode = $this->makeNode(className: 'App\\Domain\\OrderRepositoryInterface'); - $this->assertTrue($mustBeImplementedInterfaceRule->appliesTo($classNode)); + $this->assertTrue($mustBeUsedInterfaceRule->appliesTo($classNode)); } public function testDoesNotApplyToNonMatchingPattern(): void { - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule( + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule( layer: 'Domain', classNamePattern: '/Repository$/' ); - $classNode = $this->makeNode(className: 'App\\Domain\\OrderRepositoryInterface'); + $classNode = $this->makeNode(className: 'App\\Domain\\OrderRepositoryInterface'); - $this->assertFalse($mustBeImplementedInterfaceRule->appliesTo($classNode)); + $this->assertFalse($mustBeUsedInterfaceRule->appliesTo($classNode)); } public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void @@ -183,9 +183,9 @@ public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void . "interface UnusedInterface extends ArrayAccess\n{\n}\n" ); - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); - $this->assertTrue($mustBeImplementedInterfaceRule->fix(new RuleViolation( + $this->assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( message: 'Interface [App\\UnusedInterface] must be implemented by a class' . ' or extended by another interface', file: $file, @@ -208,9 +208,9 @@ public function testFixKeepsFileWhenDeclareBlockContainsExecutableCode(): void "assertTrue($mustBeImplementedInterfaceRule->fix(new RuleViolation( + $this->assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( message: 'Interface [UnusedInterface] must be implemented by a class' . ' or extended by another interface', file: $file, @@ -237,9 +237,9 @@ public function testFixKeepsFileWhenOtherCodeRemains(): void . "interface UnusedInterface\n{\n}\n\nfinal class Order\n{\n}\n" ); - $mustBeImplementedInterfaceRule = new MustBeImplementedInterfaceRule(layer: 'Domain'); + $mustBeUsedInterfaceRule = new MustBeUsedInterfaceRule(layer: 'Domain'); - $this->assertTrue($mustBeImplementedInterfaceRule->fix(new RuleViolation( + $this->assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( message: 'Interface [App\\UnusedInterface] must be implemented by a class' . ' or extended by another interface', file: $file, diff --git a/tests/Rule/Class_/MustBeUsedTraitRuleTest.php b/tests/Rule/Class_/MustBeUsedTraitRuleTest.php index a8ec27e0..29b74332 100644 --- a/tests/Rule/Class_/MustBeUsedTraitRuleTest.php +++ b/tests/Rule/Class_/MustBeUsedTraitRuleTest.php @@ -29,7 +29,7 @@ private function makeNode( bool $isInterface = false, bool $isTrait = true, bool $isEnum = false, - bool $isUsed = false, + bool $isReferenced = false, ): ClassNode { return new ClassNode( className: $className, @@ -43,14 +43,14 @@ className: $className, isReadonly: false, isTrait: $isTrait, isEnum: $isEnum, - isUsed: $isUsed, + isReferenced: $isReferenced, ); } public function testPassesWhenTraitIsUsed(): void { $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); - $classNode = $this->makeNode(isUsed: true); + $classNode = $this->makeNode(isReferenced: true); $this->assertNotInstanceOf(RuleViolation::class, $mustBeUsedTraitRule->evaluate($classNode)); } @@ -58,7 +58,7 @@ public function testPassesWhenTraitIsUsed(): void public function testViolatesWhenTraitIsNotUsed(): void { $mustBeUsedTraitRule = new MustBeUsedTraitRule(layer: 'Domain'); - $classNode = $this->makeNode(isUsed: false); + $classNode = $this->makeNode(isReferenced: false); $violation = $mustBeUsedTraitRule->evaluate($classNode); From 3e46f0b5ee9a6c40041830e494325aac4a5bb4f5 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 10:32:46 +0700 Subject: [PATCH 10/19] marker note --- src/Rule/UsedInterfaceAwareRuleInterface.php | 16 ++++++++-------- src/Rule/UsedTraitAwareRuleInterface.php | 7 ++++--- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/Rule/UsedInterfaceAwareRuleInterface.php b/src/Rule/UsedInterfaceAwareRuleInterface.php index ccf05bfb..ca6a07a7 100644 --- a/src/Rule/UsedInterfaceAwareRuleInterface.php +++ b/src/Rule/UsedInterfaceAwareRuleInterface.php @@ -5,15 +5,15 @@ namespace Boundwize\StructArmed\Rule; /** - * Marker for rules whose evaluation depends on whether an interface is - * implemented by a scanned class (directly or through inheritance) or extended - * by another scanned interface. When at least one active rule implements this - * marker, the analyser flags each ClassNode's $isImplemented before rules are - * evaluated, so implementers can read $classNode->isImplemented. + * Marker for rules whose evaluation depends on whether an interface is used + * within the scanned paths — implemented by a scanned class (directly or + * through inheritance), extended by another scanned interface, or referenced + * as a dependency. When at least one active rule implements this marker, the + * analyser flags each ClassNode's $isImplemented and $isReferenced before + * rules are evaluated, so implementers can read both flags. * - * Trade-off: only usage within the scanned paths is known. An interface - * implemented solely by a consumer outside the scan is reported as if not - * implemented. + * Trade-off: only usage within the scanned paths is known. An interface used + * solely by a consumer outside the scan is reported as if not used. */ interface UsedInterfaceAwareRuleInterface extends RuleInterface { diff --git a/src/Rule/UsedTraitAwareRuleInterface.php b/src/Rule/UsedTraitAwareRuleInterface.php index eb5284e9..e34cd028 100644 --- a/src/Rule/UsedTraitAwareRuleInterface.php +++ b/src/Rule/UsedTraitAwareRuleInterface.php @@ -5,9 +5,10 @@ namespace Boundwize\StructArmed\Rule; /** - * Marker for rules whose evaluation depends on whether a trait is used by - * another scanned class-like (class, trait, or enum). When at least one active - * rule implements this marker, the analyser flags each ClassNode's $isReferenced + * Marker for rules whose evaluation depends on whether a trait is used within + * the scanned paths — used by another scanned class-like (class, trait, or + * enum) or referenced as a dependency. When at least one active rule + * implements this marker, the analyser flags each ClassNode's $isReferenced * before rules are evaluated, so implementers can read $classNode->isReferenced. * * Trade-off: only usage within the scanned paths is known. A trait used solely From 53491740c2f535536ad910e0a3b3fc60a3c3ba15 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 23:01:22 +0700 Subject: [PATCH 11/19] add ExtendedClassMustBeAbstractOrReferencedRule --- docs/available-rules.md | 3 +- docs/presets.md | 2 +- src/Analyser/Analyser.php | 22 +- src/Analyser/ClassCollector.php | 64 +++++ src/Preset/Presets/YagniPreset.php | 12 +- .../Class_/AddAbstractClassVisitor.php | 37 +++ ...dedClassMustBeAbstractOrReferencedRule.php | 70 +++++ tests/Analyser/AnalyserTest.php | 102 ++++++++ tests/Analyser/ClassCollectorTest.php | 52 ++++ tests/Preset/PresetTest.php | 5 + ...lassMustBeAbstractOrReferencedRuleTest.php | 243 ++++++++++++++++++ .../Class_/AddAbstractClassVisitorTest.php | 80 ++++++ 12 files changed, 686 insertions(+), 6 deletions(-) create mode 100644 src/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitor.php create mode 100644 src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrReferencedRule.php create mode 100644 tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php create mode 100644 tests/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitorTest.php diff --git a/docs/available-rules.md b/docs/available-rules.md index 4f334f4b..70f9082c 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -78,6 +78,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `ClassNameMustBeStudlyCapsRule` | `new ClassNameMustBeStudlyCapsRule(layer: 'Source')` | Class names use StudlyCaps. | | `ClassNameMustHaveSuffixRule` | `new ClassNameMustHaveSuffixRule(layer: 'Controller', suffix: 'Controller')` | Classes in a layer have the required suffix. | | `ClassNameMustNotHavePrefixRule` | `new ClassNameMustNotHavePrefixRule(layer: 'Model', prefix: 'Model')` | Classes in a layer do not use a forbidden prefix. | +| `ExtendedClassMustBeAbstractOrReferencedRule` | `new ExtendedClassMustBeAbstractOrReferencedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also referenced as a dependency (instantiated, type-hinted, `::class`, a class-name string, ...). Supports `--fix` by adding the `abstract` modifier. | | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | @@ -94,7 +95,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. `classNamePattern` and `excludePattern` are regular expressions matched against the fully-qualified class name. -`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `MustBeFinalRule`, `MustBeUsedInterfaceRule`, `MustBeUsedAbstractClassRule`, `MustBeUsedTraitRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` class modifier, remove unused interfaces, abstract classes, and traits (deleting their file when only `declare`/`namespace`/`use` boilerplate remains), and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. +`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `ExtendedClassMustBeAbstractOrReferencedRule`, `MustBeFinalRule`, `MustBeUsedInterfaceRule`, `MustBeUsedAbstractClassRule`, `MustBeUsedTraitRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` or `abstract` class modifier, remove unused interfaces, abstract classes, and traits (deleting their file when only `declare`/`namespace`/`use` boilerplate remains), and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. ## Layer Rules diff --git a/docs/presets.md b/docs/presets.md index 1533a816..26fd8e62 100644 --- a/docs/presets.md +++ b/docs/presets.md @@ -25,7 +25,7 @@ StructArmed ships with presets for common PHP standards and architecture styles. | `Preset::PSR4()` | Verifies configured source paths exist in composer.json `autoload` or `autoload-dev` PSR-4 mappings | | `Preset::DDD()` | Layer isolation, entity/VO/repository/event/service conventions | | `Preset::MVC()` | Layer isolation, thin controllers, model/view/service rules | -| `Preset::YAGNI()` | Speculative-abstraction cleanup: interfaces must be implemented by a class or extended by another interface, abstract classes must be extended, traits must be used within the scanned paths — a dependency reference (type hint, `instanceof`, `::class`, static call, a class-name string, ...) also counts as usage. All three rules support `--fix` by removing the unused declaration | +| `Preset::YAGNI()` | Speculative-abstraction cleanup: interfaces must be implemented by a class or extended by another interface, abstract classes must be extended, traits must be used, and extended classes that are never referenced directly must be abstract — a dependency reference (type hint, `instanceof`, `::class`, static call, a class-name string, ...) also counts as usage within the scanned paths. All rules support `--fix`, removing the unused declaration or adding the `abstract` modifier | ## Initialize Presets diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index ca168c71..5d5d98ac 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -762,12 +762,30 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e $used[strtolower($trait)] = true; } - $selfKey = strtolower($classNode->className); + // A node's own inheritance-clause names (and the imports that + // exist for them) are structural relations, not value references: + // extending a class must not count as "referencing" it, or an + // extended-but-unreferenced class could never be told apart from + // a genuinely referenced one. Structural usage is covered by the + // dedicated extended/implemented/trait marking, and a child that + // instantiates its parent is covered by the instantiation + // collection in the file-level references. + $excludedKeys = [strtolower($classNode->className) => true]; + + if ($classNode->extends !== null) { + $excludedKeys[strtolower($classNode->extends)] = true; + } + + foreach ([$classNode->implements, $classNode->interfaceExtends, $classNode->traits] as $clauseNames) { + foreach ($clauseNames as $clauseName) { + $excludedKeys[strtolower($clauseName)] = true; + } + } foreach ($classNode->dependencies as $dependency) { $dependencyKey = strtolower($dependency); - if ($dependencyKey !== $selfKey) { + if (! isset($excludedKeys[$dependencyKey])) { $used[$dependencyKey] = true; } } diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index e831be4d..b6c07310 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -18,6 +18,7 @@ use PhpParser\Node\Expr\Include_; use PhpParser\Node\Expr\Isset_; use PhpParser\Node\Expr\List_; +use PhpParser\Node\Expr\New_; use PhpParser\Node\Expr\Print_; use PhpParser\Node\Expr\Ternary; use PhpParser\Node\Expr\Variable; @@ -56,6 +57,7 @@ use function array_unique; use function array_values; use function count; +use function end; use function in_array; use function is_string; use function preg_match; @@ -102,6 +104,15 @@ final class ClassCollector extends NodeVisitorAbstract /** @var list */ private array $currentFileReferences = []; + /** + * Stack of named class-likes currently being entered, so `new self`, + * `new static`, and `new parent` instantiations can be resolved to the + * class names they target. + * + * @var list + */ + private array $activeClassLikeScopes = []; + private string $currentFile = ''; /** @var list */ @@ -255,6 +266,7 @@ traits: $this->collectTraits($node), $this->fileClassLikes[] = $node; array_pop($this->activeClassLikeAnalyses); + array_pop($this->activeClassLikeScopes); return null; } @@ -275,6 +287,7 @@ public function afterTraverse(array $nodes): null $this->classLikeAnalysis = []; $this->classLikeMethods = []; $this->activeClassLikeAnalyses = []; + $this->activeClassLikeScopes = []; $this->activeMethodIds = []; $this->methodClassLikeAnalyses = []; @@ -291,6 +304,12 @@ private function startClassLikeAnalysis(ClassLike $classLike): void $this->classLikeAnalysis[$classLikeId] = $classLikeAnalysis; $this->activeClassLikeAnalyses[] = $classLikeAnalysis; + $this->activeClassLikeScopes[] = [ + 'name' => $this->resolveClassName($classLike), + 'extends' => $classLike instanceof Class_ && $classLike->extends instanceof Name + ? $classLike->extends->toString() + : null, + ]; foreach ($classLike->getMethods() as $classMethod) { $methodId = spl_object_id($classMethod); @@ -343,6 +362,17 @@ private function collectNodeAnalysis(Node $node): void return; } + // Instantiations always count as references, wherever they appear: + // `new` on an abstracted or removed class is fatal, so the referenced + // class must stay alive and concrete. Inheritance-clause names are + // excluded from the dependency-based reference scan, which makes this + // the signal that protects a parent class its own child instantiates. + if ($node instanceof New_) { + $this->collectInstantiation($node); + + return; + } + if ($this->activeClassLikeAnalyses === []) { // Outside any named class-like scope — procedural functions, // top-level statements, top-level anonymous class bodies — a @@ -461,6 +491,40 @@ private function collectNodeAnalysis(Node $node): void } } + private function collectInstantiation(New_ $new): void + { + $class = $new->class; + + if ($class instanceof FullyQualified) { + $this->currentFileReferences[] = $class->toString(); + + return; + } + + // Dynamic (`new $class`) and anonymous (`new class {}`) instantiations + // carry no resolvable name here; dynamic ones are conservatively + // covered by the class-name string collection. + if (! $class instanceof Name) { + return; + } + + // After name resolution only self, static, and parent survive as + // plain names; resolve them against the enclosing class-like scope. + $scope = end($this->activeClassLikeScopes); + + if ($scope === false) { + return; + } + + $relativeName = $class->toLowerString(); + + if ($relativeName === 'self' || $relativeName === 'static') { + $this->currentFileReferences[] = $scope['name']; + } elseif ($relativeName === 'parent' && $scope['extends'] !== null) { + $this->currentFileReferences[] = $scope['extends']; + } + } + private function addDependency(string $dependency): void { foreach ($this->activeClassLikeAnalyses as $activeClassLikeAnalysis) { diff --git a/src/Preset/Presets/YagniPreset.php b/src/Preset/Presets/YagniPreset.php index 70865257..93c0bf4a 100644 --- a/src/Preset/Presets/YagniPreset.php +++ b/src/Preset/Presets/YagniPreset.php @@ -6,6 +6,7 @@ use Boundwize\StructArmed\Architecture; use Boundwize\StructArmed\Preset\PresetInterface; +use Boundwize\StructArmed\Rule\Rules\Class_\ExtendedClassMustBeAbstractOrReferencedRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedInterfaceRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedTraitRule; @@ -13,8 +14,9 @@ /** * YAGNI ("You Aren't Gonna Need It") preset: reports speculative abstractions * nothing in the scanned paths needs — interfaces no class implements and no - * interface extends, abstract classes no class extends, and traits no - * class-like uses. + * interface extends, abstract classes no class extends, traits no class-like + * uses, and extended classes that are never referenced directly and so should + * be abstract. * * Trade-off: only usage within the scanned paths is known. Abstractions that * exist for consumers outside the scan (e.g. a published library's extension @@ -30,6 +32,8 @@ public const TRAIT_MUST_BE_USED = 'yagni.trait.must_be_used'; + public const EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED = 'yagni.extended_class.must_be_abstract_or_referenced'; + /** * @param list|null $sourcePaths */ @@ -55,5 +59,9 @@ public function apply(Architecture $architecture): void self::TRAIT_MUST_BE_USED, new MustBeUsedTraitRule($layerName) ); + $architecture->rule( + self::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED, + new ExtendedClassMustBeAbstractOrReferencedRule($layerName) + ); } } diff --git a/src/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitor.php b/src/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitor.php new file mode 100644 index 00000000..e3ed3d62 --- /dev/null +++ b/src/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitor.php @@ -0,0 +1,37 @@ +isAbstract() || $node->isFinal() || $node->isAnonymous()) { + return null; + } + + if ($node->namespacedName?->toString() !== $this->className) { + return null; + } + + $node->flags |= Modifiers::ABSTRACT; + + return $node; + } +} diff --git a/src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrReferencedRule.php b/src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrReferencedRule.php new file mode 100644 index 00000000..b31a150f --- /dev/null +++ b/src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrReferencedRule.php @@ -0,0 +1,70 @@ +isClass() || $classNode->isAbstract) { + return false; + } + + if (! $classNode->isInLayer($this->layer)) { + return false; + } + + if ($this->classNamePattern !== null) { + return $classNode->nameMatches($this->classNamePattern, isFullName: true); + } + + return true; + } + + public function evaluate(ClassNode $classNode): ?RuleViolation + { + if (! $classNode->isExtended) { + return null; + } + + // A dependency reference (instantiation, type hint, ::class, a + // class-name string, ...) means the class is consumed as a value, so + // making it abstract could break the referencing code. + if ($classNode->isReferenced) { + return null; + } + + return new RuleViolation( + message: sprintf( + 'Extended class [%s] must be declared abstract or referenced as a dependency', + $classNode->className + ), + file: $classNode->file, + line: $classNode->line, + className: $classNode->className, + layer: $classNode->layer, + ); + } + + protected function createFixerVisitor(RuleViolation $ruleViolation): AddAbstractClassVisitor + { + return new AddAbstractClassVisitor($ruleViolation->className); + } +} diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index 92d4edd8..62ba48cb 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -469,6 +469,108 @@ public function testYagniRulesIgnoreSelfReferences(): void $this->assertSame('App\UnusedTrait', $traitViolations[0]->className); } + public function testExtendedClassMustBeAbstractOrReferencedRuleFlagsOnlyUnreferencedParent(): void + { + $basePath = $this->makeTempProject([ + 'src/BaseRepository.php' => ' 'withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + + // BaseRepository is only ever used as a parent class; extending it + // does not count as a reference, so it should be abstract. + $this->assertCount(1, $violations); + $this->assertSame('App\BaseRepository', $violations[0]->className); + } + + public function testExtendedClassMustBeAbstractOrReferencedRulePassesWhenParentIsInstantiated(): void + { + $factory = 'makeTempProject([ + 'src/BaseRepository.php' => ' ' $factory, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + + $this->assertCount(0, $violations); + } + + public function testExtendedClassMustBeAbstractOrReferencedRulePassesWhenChildInstantiatesParent(): void + { + // The extends clause itself must not count as a reference, but the + // child's `new BaseRepository()` must: making the parent abstract + // would fatal at that instantiation. + $child = 'makeTempProject([ + 'src/BaseRepository.php' => ' $child, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + + $this->assertCount(0, $violations); + } + + public function testExtendedClassMustBeAbstractOrReferencedRulePassesOnSelfAndParentInstantiation(): void + { + // `new self()` resolves to the class itself even when called through a + // subclass, and `new parent()` resolves to the extended class — both + // would fatal if the target became abstract. + $connection = 'makeTempProject([ + 'src/Connection.php' => $connection, + 'src/ConnectionPool.php' => $pool, + 'src/TunedPool.php' => $tuned, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + + // Connection is protected by both `new self()` and `new parent()`; + // ConnectionPool is extended but never referenced, so only it is + // reported. + $this->assertCount(1, $violations); + $this->assertSame('App\ConnectionPool', $violations[0]->className); + } + public function testYagniRulesDoNotFlagAbstractionsReferencedByClassNameString(): void { $checker = 'assertSame([], $classCollector->getFileReferences()); } + public function testCollectsInstantiationsAsFileReferences(): void + { + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Service']], + $classCollector->getFileReferences() + ); + } + + public function testResolvesSelfStaticAndParentInstantiations(): void + { + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Repository', 'App\BaseRepository']], + $classCollector->getFileReferences() + ); + } + + public function testDoesNotCollectDynamicOrAnonymousInstantiations(): void + { + $code = 'makeCollector($code); + + // `new $class` has no resolvable name (class-name strings cover it), + // and anonymous classes are tracked as AnonymousClassNodes. + $this->assertSame([], $classCollector->getFileReferences()); + } + + public function testIgnoresRelativeInstantiationOutsideClassScope(): void + { + // `new self` outside a class parses but cannot be resolved to a name; + // PHP itself rejects it at runtime. + $classCollector = $this->makeCollector('assertSame([], $classCollector->getFileReferences()); + } + public function testCollectsFinalClass(): void { $classNode = $this->collect('assertInstanceOf( + ExtendedClassMustBeAbstractOrReferencedRule::class, + $rules[YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED] ?? null + ); } public function testYagniPresetUsesComposerSourcePathsByDefault(): void diff --git a/tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php b/tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php new file mode 100644 index 00000000..57e1ce00 --- /dev/null +++ b/tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php @@ -0,0 +1,243 @@ +makeNode(isExtended: true); + + $violation = $extendedClassMustBeAbstractOrReferencedRule->evaluate($classNode); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertStringContainsString('must be declared abstract', $violation->message); + } + + public function testPassesWhenClassIsNotExtended(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isExtended: false); + + $this->assertNotInstanceOf( + RuleViolation::class, + $extendedClassMustBeAbstractOrReferencedRule->evaluate($classNode) + ); + } + + public function testPassesWhenExtendedClassIsReferenced(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isExtended: true, isReferenced: true); + + $this->assertNotInstanceOf( + RuleViolation::class, + $extendedClassMustBeAbstractOrReferencedRule->evaluate($classNode) + ); + } + + public function testIsExtendedClassAware(): void + { + $this->assertInstanceOf( + ExtendedClassAwareRuleInterface::class, + new ExtendedClassMustBeAbstractOrReferencedRule(layer: 'Domain') + ); + } + + public function testIsFixable(): void + { + $this->assertInstanceOf( + FixableInterface::class, + new ExtendedClassMustBeAbstractOrReferencedRule(layer: 'Domain') + ); + } + + public function testCreatesAddAbstractClassFixerVisitor(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $reflectionMethod = new ReflectionMethod( + $extendedClassMustBeAbstractOrReferencedRule, + 'createFixerVisitor' + ); + $addAbstractClassVisitor = $reflectionMethod->invoke( + $extendedClassMustBeAbstractOrReferencedRule, + new RuleViolation( + message: 'Extended class [App\\BaseRepository] must be declared abstract' + . ' or referenced as a dependency', + file: '/src/BaseRepository.php', + line: 1, + className: 'App\\BaseRepository', + layer: 'Domain', + ) + ); + + $this->assertInstanceOf(AddAbstractClassVisitor::class, $addAbstractClassVisitor); + } + + public function testDoesNotApplyToWrongLayer(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(layer: 'Infrastructure'); + + $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToAbstractClasses(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isAbstract: true); + + $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToInterfaces(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isInterface: true); + + $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToTraits(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isTrait: true); + + $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToEnums(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isEnum: true); + + $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testAppliesToMatchingPattern(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain', + classNamePattern: '/Repository$/' + ); + $classNode = $this->makeNode(); + + $this->assertTrue($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToNonMatchingPattern(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain', + classNamePattern: '/Service$/' + ); + $classNode = $this->makeNode(); + + $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testAppliesToLayerWhenNoPatternConfigured(): void + { + $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(); + + $this->assertTrue($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); + } + + public function testFixDeclaresClassAbstract(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-extended'); + $file = $temporaryDirectory . '/SomeBaseClass.php'; + + file_put_contents( + $file, + "assertTrue($extendedClassMustBeAbstractOrReferencedRule->fix(new RuleViolation( + message: 'Extended class [App\\SomeBaseClass] must be declared abstract' + . ' or referenced as a dependency', + file: $file, + line: 7, + className: 'App\\SomeBaseClass', + layer: 'Domain', + ))); + $this->assertFileExists($file); + $this->assertStringContainsString( + 'abstract class SomeBaseClass', + (string) file_get_contents($file) + ); + } +} diff --git a/tests/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitorTest.php b/tests/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitorTest.php new file mode 100644 index 00000000..758032c5 --- /dev/null +++ b/tests/Rule/Fixer/PhpParser/Class_/AddAbstractClassVisitorTest.php @@ -0,0 +1,80 @@ +namespacedName = new Name('App\\BaseRepository'); + + (new NodeTraverser($addAbstractClassVisitor))->traverse([$class]); + + $this->assertSame(Modifiers::ABSTRACT, $class->flags); + } + + public function testDoesNotChangeNonClassNode(): void + { + $addAbstractClassVisitor = new AddAbstractClassVisitor('App\\BaseRepository'); + + $this->assertNotInstanceOf(Node::class, $addAbstractClassVisitor->enterNode(new ClassMethod('save'))); + } + + public function testDoesNotChangeAlreadyAbstractClass(): void + { + $class = new Class_('BaseRepository', ['flags' => Modifiers::ABSTRACT]); + $addAbstractClassVisitor = new AddAbstractClassVisitor('App\\BaseRepository'); + $class->namespacedName = new Name('App\\BaseRepository'); + + (new NodeTraverser($addAbstractClassVisitor))->traverse([$class]); + + $this->assertSame(Modifiers::ABSTRACT, $class->flags); + } + + public function testDoesNotChangeFinalClass(): void + { + $class = new Class_('BaseRepository', ['flags' => Modifiers::FINAL]); + $addAbstractClassVisitor = new AddAbstractClassVisitor('App\\BaseRepository'); + $class->namespacedName = new Name('App\\BaseRepository'); + + (new NodeTraverser($addAbstractClassVisitor))->traverse([$class]); + + $this->assertSame(Modifiers::FINAL, $class->flags); + } + + public function testDoesNotChangeDifferentClass(): void + { + $class = new Class_('BaseRepository'); + $addAbstractClassVisitor = new AddAbstractClassVisitor('App\\BaseRepository'); + $class->namespacedName = new Name('App\\OtherRepository'); + + (new NodeTraverser($addAbstractClassVisitor))->traverse([$class]); + + $this->assertSame(0, $class->flags); + } + + public function testDoesNotChangeAnonymousClass(): void + { + $class = new Class_(null); + $addAbstractClassVisitor = new AddAbstractClassVisitor('App\\BaseRepository'); + + (new NodeTraverser($addAbstractClassVisitor))->traverse([$class]); + + $this->assertSame(0, $class->flags); + } +} From da14a5e2f929417a49b0e518c31ced627a501741 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 23:41:44 +0700 Subject: [PATCH 12/19] make instantiation check instead --- docs/available-rules.md | 4 +- docs/presets.md | 2 +- src/Analyser/Analyser.php | 72 ++++-- src/Analyser/ClassCollector.php | 209 ++++++++++++--- src/Analyser/ClassNode.php | 12 + src/Analyser/ClassNodeExtractor.php | 1 + src/Analyser/ExtractionResult.php | 3 + src/Analyser/Parallel/ClassNodeWorker.php | 2 + .../Parallel/ParallelClassNodeExtractor.php | 33 ++- src/Cache/AnalysisResultCache.php | 30 ++- src/Preset/Presets/YagniPreset.php | 13 +- ...ClassMustBeAbstractOrInstantiatedRule.php} | 13 +- tests/Analyser/AnalyserTest.php | 84 +++++- tests/Analyser/ClassCollectorTest.php | 70 ++++- tests/Analyser/ClassNodeTest.php | 25 +- .../ParallelClassNodeExtractorTest.php | 39 +++ tests/Cache/AnalysisResultCacheTest.php | 30 +++ tests/Preset/PresetTest.php | 6 +- ...ustBeAbstractOrInstantiatedRuleFixTest.php | 51 ++++ ...ssMustBeAbstractOrInstantiatedRuleTest.php | 225 ++++++++++++++++ ...lassMustBeAbstractOrReferencedRuleTest.php | 243 ------------------ .../MustBeUsedAbstractClassRuleFixTest.php | 43 ++++ .../MustBeUsedAbstractClassRuleTest.php | 27 -- .../Class_/MustBeUsedInterfaceRuleFixTest.php | 109 ++++++++ .../Class_/MustBeUsedInterfaceRuleTest.php | 93 ------- .../Class_/MustBeUsedTraitRuleFixTest.php | 43 ++++ tests/Rule/Class_/MustBeUsedTraitRuleTest.php | 27 -- 27 files changed, 1025 insertions(+), 484 deletions(-) rename src/Rule/Rules/Class_/{ExtendedClassMustBeAbstractOrReferencedRule.php => ExtendedClassMustBeAbstractOrInstantiatedRule.php} (80%) create mode 100644 tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleFixTest.php create mode 100644 tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleTest.php delete mode 100644 tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php create mode 100644 tests/Rule/Class_/MustBeUsedAbstractClassRuleFixTest.php create mode 100644 tests/Rule/Class_/MustBeUsedInterfaceRuleFixTest.php create mode 100644 tests/Rule/Class_/MustBeUsedTraitRuleFixTest.php diff --git a/docs/available-rules.md b/docs/available-rules.md index 70f9082c..ac46e426 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -78,7 +78,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `ClassNameMustBeStudlyCapsRule` | `new ClassNameMustBeStudlyCapsRule(layer: 'Source')` | Class names use StudlyCaps. | | `ClassNameMustHaveSuffixRule` | `new ClassNameMustHaveSuffixRule(layer: 'Controller', suffix: 'Controller')` | Classes in a layer have the required suffix. | | `ClassNameMustNotHavePrefixRule` | `new ClassNameMustNotHavePrefixRule(layer: 'Model', prefix: 'Model')` | Classes in a layer do not use a forbidden prefix. | -| `ExtendedClassMustBeAbstractOrReferencedRule` | `new ExtendedClassMustBeAbstractOrReferencedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also referenced as a dependency (instantiated, type-hinted, `::class`, a class-name string, ...). Supports `--fix` by adding the `abstract` modifier. | +| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count. Supports `--fix` by adding the `abstract` modifier. | | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | @@ -95,7 +95,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. `classNamePattern` and `excludePattern` are regular expressions matched against the fully-qualified class name. -`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `ExtendedClassMustBeAbstractOrReferencedRule`, `MustBeFinalRule`, `MustBeUsedInterfaceRule`, `MustBeUsedAbstractClassRule`, `MustBeUsedTraitRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` or `abstract` class modifier, remove unused interfaces, abstract classes, and traits (deleting their file when only `declare`/`namespace`/`use` boilerplate remains), and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. +`Psr4DirectoryExistsRule`, `Psr1PhpTagsRule`, `Psr1Utf8WithoutBomRule`, `ExtendedClassMustBeAbstractOrInstantiatedRule`, `MustBeFinalRule`, `MustBeUsedInterfaceRule`, `MustBeUsedAbstractClassRule`, `MustBeUsedTraitRule`, `MustDeclareConstantVisibilityRule`, `MustDeclareMethodVisibilityRule`, and `MustDeclarePropertyVisibilityRule` implement `Boundwize\StructArmed\Rule\FixableInterface`, so StructArmed can automatically remove PSR-4 mappings for missing directories, normalize invalid PHP opening tags, remove UTF-8 byte order marks, add the `final` or `abstract` class modifier, remove unused interfaces, abstract classes, and traits (deleting their file when only `declare`/`namespace`/`use` boilerplate remains), and add missing constant, method, or property visibility modifiers when you run `vendor/bin/structarmed analyse --fix`. ## Layer Rules diff --git a/docs/presets.md b/docs/presets.md index 26fd8e62..c9b0ce0a 100644 --- a/docs/presets.md +++ b/docs/presets.md @@ -25,7 +25,7 @@ StructArmed ships with presets for common PHP standards and architecture styles. | `Preset::PSR4()` | Verifies configured source paths exist in composer.json `autoload` or `autoload-dev` PSR-4 mappings | | `Preset::DDD()` | Layer isolation, entity/VO/repository/event/service conventions | | `Preset::MVC()` | Layer isolation, thin controllers, model/view/service rules | -| `Preset::YAGNI()` | Speculative-abstraction cleanup: interfaces must be implemented by a class or extended by another interface, abstract classes must be extended, traits must be used, and extended classes that are never referenced directly must be abstract — a dependency reference (type hint, `instanceof`, `::class`, static call, a class-name string, ...) also counts as usage within the scanned paths. All rules support `--fix`, removing the unused declaration or adding the `abstract` modifier | +| `Preset::YAGNI()` | Speculative-abstraction cleanup: interfaces must be implemented by a class or extended by another interface, abstract classes must be extended, traits must be used, and extended classes that are never instantiated must be abstract — a dependency reference (type hint, `instanceof`, `::class`, static call, a class-name string, ...) also counts as usage within the scanned paths, while only instantiation (`new X`, `new self`/`static`/`parent`, or a dynamic `new $class` resolvable from a constant expression) keeps an extended class concrete. All rules support `--fix`, removing the unused declaration or adding the `abstract` modifier | ## Initialize Presets diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 5d5d98ac..71831ff4 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -762,30 +762,12 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e $used[strtolower($trait)] = true; } - // A node's own inheritance-clause names (and the imports that - // exist for them) are structural relations, not value references: - // extending a class must not count as "referencing" it, or an - // extended-but-unreferenced class could never be told apart from - // a genuinely referenced one. Structural usage is covered by the - // dedicated extended/implemented/trait marking, and a child that - // instantiates its parent is covered by the instantiation - // collection in the file-level references. - $excludedKeys = [strtolower($classNode->className) => true]; - - if ($classNode->extends !== null) { - $excludedKeys[strtolower($classNode->extends)] = true; - } - - foreach ([$classNode->implements, $classNode->interfaceExtends, $classNode->traits] as $clauseNames) { - foreach ($clauseNames as $clauseName) { - $excludedKeys[strtolower($clauseName)] = true; - } - } + $selfKey = strtolower($classNode->className); foreach ($classNode->dependencies as $dependency) { $dependencyKey = strtolower($dependency); - if (! isset($excludedKeys[$dependencyKey])) { + if ($dependencyKey !== $selfKey) { $used[$dependencyKey] = true; } } @@ -808,10 +790,28 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e } } + // Instantiations (`new X`, with self/static/parent already resolved) + // are the one usage that requires a class to stay concrete, so they + // are tracked apart from plain references. No self-exclusion here: a + // class instantiating itself cannot become abstract either. + $instantiated = []; + + foreach ($extractionResult->fileInstantiations as $instantiations) { + foreach ($instantiations as $instantiation) { + $instantiated[strtolower($instantiation)] = true; + } + } + foreach ($classNodes as $classNode) { - if (isset($used[strtolower($classNode->className)])) { + $classNameKey = strtolower($classNode->className); + + if (isset($used[$classNameKey]) || isset($instantiated[$classNameKey])) { $classNode->setReferenced(true); } + + if (isset($instantiated[$classNameKey])) { + $classNode->setInstantiated(true); + } } } @@ -980,6 +980,7 @@ private function collectClassNodes( $fileAnalyses = []; $anonymousClassNodes = []; $fileReferences = []; + $fileInstantiations = []; $filesToParse = []; foreach ($files as $file) { @@ -1006,6 +1007,10 @@ private function collectClassNodes( $fileReferences[$file] = $cachedResult['fileReferences']; } + if ($cachedResult['fileInstantiations'] !== []) { + $fileInstantiations[$file] = $cachedResult['fileInstantiations']; + } + $fileAnalyses[$file] = $cachedResult['fileAnalysis']; continue; @@ -1032,6 +1037,10 @@ private function collectClassNodes( if ($cachedResult['fileReferences'] !== []) { $fileReferences[$file] = $cachedResult['fileReferences']; } + + if ($cachedResult['fileInstantiations'] !== []) { + $fileInstantiations[$file] = $cachedResult['fileInstantiations']; + } } $progressHandler?->start(count($filesToParse)); @@ -1039,7 +1048,13 @@ private function collectClassNodes( if ($filesToParse === []) { $progressHandler?->finish(); - return new ExtractionResult($classNodes, $fileAnalyses, $anonymousClassNodes, $fileReferences); + return new ExtractionResult( + $classNodes, + $fileAnalyses, + $anonymousClassNodes, + $fileReferences, + $fileInstantiations, + ); } $options = $analyserOptions ?? AnalyserOptions::parallel(); @@ -1086,6 +1101,10 @@ private function collectClassNodes( $fileReferences[$file] = $parsedFileReferences; } + foreach ($parsedResult->fileInstantiations as $file => $parsedFileInstantiations) { + $fileInstantiations[$file] = $parsedFileInstantiations; + } + foreach ($classNodesByFile as $fileToParse => $fileClassNodes) { $this->analysisResultCache?->storeClassNodes( $fileToParse, @@ -1094,12 +1113,19 @@ private function collectClassNodes( $fileAnalyses[$fileToParse] ?? null, $anonymousClassNodesByFile[$fileToParse] ?? [], $fileReferences[$fileToParse] ?? [], + $fileInstantiations[$fileToParse] ?? [], ); } $progressHandler?->finish(); - return new ExtractionResult($classNodes, $fileAnalyses, $anonymousClassNodes, $fileReferences); + return new ExtractionResult( + $classNodes, + $fileAnalyses, + $anonymousClassNodes, + $fileReferences, + $fileInstantiations, + ); } /** diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index b6c07310..3e78175a 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -6,11 +6,17 @@ use Boundwize\StructArmed\LayerResolver\LayerResolverInterface; use Boundwize\StructArmed\Util\PhpParser\VisibilityFlagChecker; +use PhpParser\ConstExprEvaluationException; +use PhpParser\ConstExprEvaluator; use PhpParser\Node; +use PhpParser\Node\Expr; +use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\BinaryOp\BooleanAnd; use PhpParser\Node\Expr\BinaryOp\BooleanOr; +use PhpParser\Node\Expr\BinaryOp\Concat; use PhpParser\Node\Expr\BinaryOp\LogicalAnd; use PhpParser\Node\Expr\BinaryOp\LogicalOr; +use PhpParser\Node\Expr\ClassConstFetch; use PhpParser\Node\Expr\Empty_; use PhpParser\Node\Expr\Eval_; use PhpParser\Node\Expr\Exit_; @@ -104,6 +110,22 @@ final class ClassCollector extends NodeVisitorAbstract /** @var list */ private array $currentFileReferences = []; + /** @var array> */ + private array $fileInstantiations = []; + + /** @var list */ + private array $currentFileInstantiations = []; + + /** + * Variables currently known to hold a constant class-name string, so + * `new $variable` instantiations can be resolved. + * + * @var array + */ + private array $variableClassNames = []; + + private readonly ConstExprEvaluator $constExprEvaluator; + /** * Stack of named class-likes currently being entered, so `new self`, * `new static`, and `new parent` instantiations can be resolved to the @@ -142,20 +164,38 @@ final class ClassCollector extends NodeVisitorAbstract public function __construct( private readonly LayerResolverInterface $layerResolver ) { + $this->constExprEvaluator = new ConstExprEvaluator(function (Expr $expr): string { + if ( + $expr instanceof ClassConstFetch + && $expr->name instanceof Identifier + && $expr->name->toLowerString() === 'class' + && $expr->class instanceof Name + ) { + $className = $this->resolveClassLikeName($expr->class); + + if ($className !== null) { + return $className; + } + } + + throw new ConstExprEvaluationException('Expression is not a resolvable class name.'); + }); } public function setCurrentFile(string $file): void { - $this->currentFile = $file; - $this->currentFileReferences = []; - $this->currentNamespaceUses = []; - $this->fileClassLikes = []; - $this->fileFunctions = []; - $this->classLikeAnalysis = []; - $this->classLikeMethods = []; - $this->activeClassLikeAnalyses = []; - $this->activeMethodIds = []; - $this->methodClassLikeAnalyses = []; + $this->currentFile = $file; + $this->currentFileReferences = []; + $this->currentFileInstantiations = []; + $this->variableClassNames = []; + $this->currentNamespaceUses = []; + $this->fileClassLikes = []; + $this->fileFunctions = []; + $this->classLikeAnalysis = []; + $this->classLikeMethods = []; + $this->activeClassLikeAnalyses = []; + $this->activeMethodIds = []; + $this->methodClassLikeAnalyses = []; } /** @return list */ @@ -182,6 +222,18 @@ public function getFileReferences(): array return $this->fileReferences; } + /** + * Class-like instantiations (`new X`, with self/static/parent resolved to + * the class names they target), per file. `new` on an abstract class is + * fatal, so these are what an extended class needs to stay concrete. + * + * @return array> + */ + public function getFileInstantiations(): array + { + return $this->fileInstantiations; + } + public function enterNode(Node $node): null { if ($node instanceof Namespace_) { @@ -243,6 +295,28 @@ public function leaveNode(Node $node): null return null; } + // Both run on leave, once the NameResolver has resolved the nested + // name nodes (e.g. Base::class inside the assigned expression). + // + // A variable assigned a constant class-name value may feed a later + // `new $variable`. + if ($node instanceof Assign) { + $this->trackVariableClassName($node); + + return null; + } + + // Instantiations are tracked separately from plain references: + // `new` on an abstract class is fatal, so instantiation is the one + // usage that requires an extended class to stay concrete — type + // hints, instanceof checks, and ::class constants all keep working + // once a class becomes abstract. + if ($node instanceof New_) { + $this->collectInstantiation($node); + + return null; + } + if (! $node instanceof ClassLike) { return null; } @@ -283,6 +357,13 @@ public function afterTraverse(array $nodes): null $this->currentFileReferences = []; } + if ($this->currentFileInstantiations !== []) { + $this->fileInstantiations[$this->currentFile] = array_values( + array_unique($this->currentFileInstantiations) + ); + $this->currentFileInstantiations = []; + } + $this->fileClassLikes = []; $this->classLikeAnalysis = []; $this->classLikeMethods = []; @@ -362,17 +443,6 @@ private function collectNodeAnalysis(Node $node): void return; } - // Instantiations always count as references, wherever they appear: - // `new` on an abstracted or removed class is fatal, so the referenced - // class must stay alive and concrete. Inheritance-clause names are - // excluded from the dependency-based reference scan, which makes this - // the signal that protects a parent class its own child instantiates. - if ($node instanceof New_) { - $this->collectInstantiation($node); - - return; - } - if ($this->activeClassLikeAnalyses === []) { // Outside any named class-like scope — procedural functions, // top-level statements, top-level anonymous class bodies — a @@ -495,34 +565,107 @@ private function collectInstantiation(New_ $new): void { $class = $new->class; - if ($class instanceof FullyQualified) { - $this->currentFileReferences[] = $class->toString(); + if ($class instanceof Name) { + $className = $this->resolveClassLikeName($class); + + if ($className !== null) { + $this->currentFileInstantiations[] = $className; + } return; } - // Dynamic (`new $class`) and anonymous (`new class {}`) instantiations - // carry no resolvable name here; dynamic ones are conservatively - // covered by the class-name string collection. - if (! $class instanceof Name) { + // Anonymous classes (`new class {}`) are tracked as + // AnonymousClassNodes; dynamic instantiations may still resolve below. + if (! $class instanceof Expr) { return; } + // `new $class` where the variable holds a constant class-name value, + // or `new (X::class)` / `new ('App\X')` class expressions. + $className = $class instanceof Variable && is_string($class->name) + ? ($this->variableClassNames[$class->name] ?? null) + : $this->resolveClassNameExpr($class); + + if ($className !== null) { + $this->currentFileInstantiations[] = $className; + } + } + + /** + * Resolve a class-like name node to a fully qualified name: either it is + * already fully qualified, or it is a self/static/parent keyword resolved + * against the enclosing class-like scope. Returns null when there is no + * scope to resolve against. + */ + private function resolveClassLikeName(Name $name): ?string + { + if ($name instanceof FullyQualified) { + return $name->toString(); + } + // After name resolution only self, static, and parent survive as - // plain names; resolve them against the enclosing class-like scope. + // plain names. $scope = end($this->activeClassLikeScopes); if ($scope === false) { - return; + return null; } - $relativeName = $class->toLowerString(); + $relativeName = $name->toLowerString(); if ($relativeName === 'self' || $relativeName === 'static') { - $this->currentFileReferences[] = $scope['name']; - } elseif ($relativeName === 'parent' && $scope['extends'] !== null) { - $this->currentFileReferences[] = $scope['extends']; + return $scope['name']; + } + + return $relativeName === 'parent' ? $scope['extends'] : null; + } + + /** + * Track `$variable = ` assignments so a + * later `new $variable` can be resolved. Over-approximation is safe: a + * recorded instantiation only keeps a class concrete or alive. + */ + private function trackVariableClassName(Assign $assign): void + { + if (! $assign->var instanceof Variable || ! is_string($assign->var->name)) { + return; + } + + $className = $this->resolveClassNameExpr($assign->expr); + + if ($className === null) { + // Reassigned to something unresolvable — drop the stale name. + unset($this->variableClassNames[$assign->var->name]); + + return; + } + + $this->variableClassNames[$assign->var->name] = $className; + } + + /** + * Evaluate a constant expression to a class-name string: 'App\X' literals, + * X::class (including self/static/parent::class), and concatenations of + * those. Anything depending on runtime values resolves to null. + */ + private function resolveClassNameExpr(Expr $expr): ?string + { + if (! $expr instanceof String_ && ! $expr instanceof ClassConstFetch && ! $expr instanceof Concat) { + return null; } + + try { + $value = $this->constExprEvaluator->evaluateSilently($expr); + } catch (ConstExprEvaluationException) { + return null; + } + + if (! is_string($value) || preg_match(self::CLASS_LIKE_STRING_PATTERN, $value) !== 1) { + return null; + } + + return $value; } private function addDependency(string $dependency): void diff --git a/src/Analyser/ClassNode.php b/src/Analyser/ClassNode.php index 4989ca20..4e8e4ce3 100644 --- a/src/Analyser/ClassNode.php +++ b/src/Analyser/ClassNode.php @@ -62,6 +62,7 @@ public function __construct( public bool $isExtended = false, public bool $isImplemented = false, public bool $isReferenced = false, + public bool $isInstantiated = false, ) { $this->layers = $layers ?: array_filter([$this->layer]); } @@ -107,6 +108,17 @@ public function setReferenced(bool $isReferenced): void $this->isReferenced = $isReferenced; } + /** + * Whether another scanned scope instantiates this class — `new X`, or a + * `new self`/`new static`/`new parent` resolving to it. Instantiation is + * the one usage that requires a class to stay concrete. Computed by the + * analyser when a usage-aware rule is active; false otherwise. + */ + public function setInstantiated(bool $isInstantiated): void + { + $this->isInstantiated = $isInstantiated; + } + public function shortName(): string { $parts = explode('\\', $this->className); diff --git a/src/Analyser/ClassNodeExtractor.php b/src/Analyser/ClassNodeExtractor.php index 9182a311..721d30c3 100644 --- a/src/Analyser/ClassNodeExtractor.php +++ b/src/Analyser/ClassNodeExtractor.php @@ -58,6 +58,7 @@ public function extract( $fileAnalyses, $classCollector->getAnonymousClassNodes(), $classCollector->getFileReferences(), + $classCollector->getFileInstantiations(), ); } } diff --git a/src/Analyser/ExtractionResult.php b/src/Analyser/ExtractionResult.php index 1ac427db..252641e6 100644 --- a/src/Analyser/ExtractionResult.php +++ b/src/Analyser/ExtractionResult.php @@ -12,12 +12,15 @@ * @param list $anonymousClassNodes * @param array> $fileReferences Class-like references made outside any * named class-like scope, per file + * @param array> $fileInstantiations Class-like instantiations (`new X`, + * with self/static/parent resolved), per file */ public function __construct( public array $classNodes, public array $fileAnalyses, public array $anonymousClassNodes = [], public array $fileReferences = [], + public array $fileInstantiations = [], ) { } } diff --git a/src/Analyser/Parallel/ClassNodeWorker.php b/src/Analyser/Parallel/ClassNodeWorker.php index 6bd3c1c4..d2c75c8f 100644 --- a/src/Analyser/Parallel/ClassNodeWorker.php +++ b/src/Analyser/Parallel/ClassNodeWorker.php @@ -64,6 +64,7 @@ public static function run(string $inputFile, string $outputFile, mixed $outputS 'fileAnalyses' => $result->fileAnalyses, 'anonymousClassNodes' => $result->anonymousClassNodes, 'fileReferences' => $result->fileReferences, + 'fileInstantiations' => $result->fileInstantiations, 'error' => null, ])); @@ -74,6 +75,7 @@ public static function run(string $inputFile, string $outputFile, mixed $outputS 'fileAnalyses' => [], 'anonymousClassNodes' => [], 'fileReferences' => [], + 'fileInstantiations' => [], 'error' => sprintf('%s: %s', $throwable::class, $throwable->getMessage()), ])); diff --git a/src/Analyser/Parallel/ParallelClassNodeExtractor.php b/src/Analyser/Parallel/ParallelClassNodeExtractor.php index 78083f24..26d97fe6 100644 --- a/src/Analyser/Parallel/ParallelClassNodeExtractor.php +++ b/src/Analyser/Parallel/ParallelClassNodeExtractor.php @@ -138,6 +138,7 @@ public function extract( $fileAnalyses = []; $anonymousClassNodes = []; $fileReferences = []; + $fileInstantiations = []; $failure = null; while ($pending !== []) { @@ -275,6 +276,36 @@ public function extract( $fileReferences[$file] = $validReferences; } + + $workerFileInstantiations = $result['fileInstantiations'] ?? []; + + if (! is_array($workerFileInstantiations)) { + throw new RuntimeException( + 'Parallel analysis worker returned invalid file instantiations.' + ); + } + + foreach ($workerFileInstantiations as $file => $instantiations) { + if (! is_string($file) || ! is_array($instantiations)) { + throw new RuntimeException( + 'Parallel analysis worker returned invalid file instantiations.' + ); + } + + $validInstantiations = []; + + foreach ($instantiations as $instantiation) { + if (! is_string($instantiation)) { + throw new RuntimeException( + 'Parallel analysis worker returned invalid file instantiations.' + ); + } + + $validInstantiations[] = $instantiation; + } + + $fileInstantiations[$file] = $validInstantiations; + } } catch (RuntimeException $runtimeException) { $failure ??= $runtimeException->getMessage(); } finally { @@ -298,7 +329,7 @@ public function extract( throw new RuntimeException($failure); } - return new ExtractionResult($nodes, $fileAnalyses, $anonymousClassNodes, $fileReferences); + return new ExtractionResult($nodes, $fileAnalyses, $anonymousClassNodes, $fileReferences, $fileInstantiations); } /** diff --git a/src/Cache/AnalysisResultCache.php b/src/Cache/AnalysisResultCache.php index 11a4164e..3f133ec6 100644 --- a/src/Cache/AnalysisResultCache.php +++ b/src/Cache/AnalysisResultCache.php @@ -189,7 +189,8 @@ private function ensureCacheInitialised(): void * @return array{ * classNodes: list, * anonymousClassNodes: list, - * fileReferences: list + * fileReferences: list, + * fileInstantiations: list * }|null */ public function loadClassNodes(string $file, string $namespace): ?array @@ -203,8 +204,14 @@ public function loadClassNodes(string $file, string $namespace): ?array $classNodes = $this->classNodesFromPayload($payload); $anonymousClassNodes = $this->anonymousClassNodesFromPayload($payload); $fileReferences = $this->fileReferencesFromPayload($payload); + $fileInstantiations = $this->fileInstantiationsFromPayload($payload); - if ($classNodes === null || $anonymousClassNodes === null || $fileReferences === null) { + if ( + $classNodes === null + || $anonymousClassNodes === null + || $fileReferences === null + || $fileInstantiations === null + ) { return null; } @@ -212,6 +219,7 @@ public function loadClassNodes(string $file, string $namespace): ?array 'classNodes' => $classNodes, 'anonymousClassNodes' => $anonymousClassNodes, 'fileReferences' => $fileReferences, + 'fileInstantiations' => $fileInstantiations, ]; } @@ -220,6 +228,7 @@ public function loadClassNodes(string $file, string $namespace): ?array * classNodes: list, * anonymousClassNodes: list, * fileReferences: list, + * fileInstantiations: list, * fileAnalysis: FileAnalysis * }|null */ @@ -234,6 +243,7 @@ public function loadClassNodesWithFileAnalysis(string $file, string $namespace): $classNodes = $this->classNodesFromPayload($payload); $anonymousClassNodes = $this->anonymousClassNodesFromPayload($payload); $fileReferences = $this->fileReferencesFromPayload($payload); + $fileInstantiations = $this->fileInstantiationsFromPayload($payload); $fileAnalysis = is_array($payload['fileAnalysis'] ?? null) ? $this->fileAnalysisFromArray($payload['fileAnalysis']) : null; @@ -242,6 +252,7 @@ public function loadClassNodesWithFileAnalysis(string $file, string $namespace): $classNodes === null || $anonymousClassNodes === null || $fileReferences === null + || $fileInstantiations === null || ! $fileAnalysis instanceof FileAnalysis ) { return null; @@ -251,6 +262,7 @@ public function loadClassNodesWithFileAnalysis(string $file, string $namespace): 'classNodes' => $classNodes, 'anonymousClassNodes' => $anonymousClassNodes, 'fileReferences' => $fileReferences, + 'fileInstantiations' => $fileInstantiations, 'fileAnalysis' => $fileAnalysis, ]; } @@ -301,6 +313,7 @@ private function classNodesFromPayload(array $payload): ?array * @param list $anonymousClassNodes * @param list $fileReferences Class-like references made outside any * named class-like scope in this file + * @param list $fileInstantiations Class-like instantiations in this file */ public function storeClassNodes( string $file, @@ -309,6 +322,7 @@ public function storeClassNodes( ?FileAnalysis $fileAnalysis = null, array $anonymousClassNodes = [], array $fileReferences = [], + array $fileInstantiations = [], ): void { $this->ensureCacheInitialised(); @@ -317,6 +331,7 @@ public function storeClassNodes( 'nodes' => array_map($this->classNodeToArray(...), $classNodes), 'anonymousClassNodes' => array_map($this->anonymousClassNodeToArray(...), $anonymousClassNodes), 'fileReferences' => $fileReferences, + 'fileInstantiations' => $fileInstantiations, ]; if ($fileAnalysis instanceof FileAnalysis) { @@ -411,6 +426,17 @@ private function fileReferencesFromPayload(array $payload): ?array return $this->isStringArray($fileReferences) ? array_values($fileReferences) : null; } + /** + * @param array $payload + * @return list|null + */ + private function fileInstantiationsFromPayload(array $payload): ?array + { + $fileInstantiations = $payload['fileInstantiations'] ?? []; + + return $this->isStringArray($fileInstantiations) ? array_values($fileInstantiations) : null; + } + /** * @return array */ diff --git a/src/Preset/Presets/YagniPreset.php b/src/Preset/Presets/YagniPreset.php index 93c0bf4a..19c69d5f 100644 --- a/src/Preset/Presets/YagniPreset.php +++ b/src/Preset/Presets/YagniPreset.php @@ -6,7 +6,7 @@ use Boundwize\StructArmed\Architecture; use Boundwize\StructArmed\Preset\PresetInterface; -use Boundwize\StructArmed\Rule\Rules\Class_\ExtendedClassMustBeAbstractOrReferencedRule; +use Boundwize\StructArmed\Rule\Rules\Class_\ExtendedClassMustBeAbstractOrInstantiatedRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedInterfaceRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedTraitRule; @@ -15,8 +15,8 @@ * YAGNI ("You Aren't Gonna Need It") preset: reports speculative abstractions * nothing in the scanned paths needs — interfaces no class implements and no * interface extends, abstract classes no class extends, traits no class-like - * uses, and extended classes that are never referenced directly and so should - * be abstract. + * uses, and extended classes that are never instantiated and so should be + * abstract. * * Trade-off: only usage within the scanned paths is known. Abstractions that * exist for consumers outside the scan (e.g. a published library's extension @@ -32,7 +32,8 @@ public const TRAIT_MUST_BE_USED = 'yagni.trait.must_be_used'; - public const EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED = 'yagni.extended_class.must_be_abstract_or_referenced'; + public const EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED = + 'yagni.extended_class.must_be_abstract_or_instantiated'; /** * @param list|null $sourcePaths @@ -60,8 +61,8 @@ public function apply(Architecture $architecture): void new MustBeUsedTraitRule($layerName) ); $architecture->rule( - self::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED, - new ExtendedClassMustBeAbstractOrReferencedRule($layerName) + self::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED, + new ExtendedClassMustBeAbstractOrInstantiatedRule($layerName) ); } } diff --git a/src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrReferencedRule.php b/src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrInstantiatedRule.php similarity index 80% rename from src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrReferencedRule.php rename to src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrInstantiatedRule.php index b31a150f..c605ce22 100644 --- a/src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrReferencedRule.php +++ b/src/Rule/Rules/Class_/ExtendedClassMustBeAbstractOrInstantiatedRule.php @@ -12,7 +12,7 @@ use function sprintf; -final readonly class ExtendedClassMustBeAbstractOrReferencedRule extends AbstractPhpParserFixableRule implements +final readonly class ExtendedClassMustBeAbstractOrInstantiatedRule extends AbstractPhpParserFixableRule implements ExtendedClassAwareRuleInterface { public function __construct( @@ -44,16 +44,17 @@ public function evaluate(ClassNode $classNode): ?RuleViolation return null; } - // A dependency reference (instantiation, type hint, ::class, a - // class-name string, ...) means the class is consumed as a value, so - // making it abstract could break the referencing code. - if ($classNode->isReferenced) { + // Only instantiation (`new X`, or `new self`/`static`/`parent` + // resolving to X) requires the class to stay concrete — type hints, + // instanceof checks, and ::class constants keep working once the + // class becomes abstract. + if ($classNode->isInstantiated) { return null; } return new RuleViolation( message: sprintf( - 'Extended class [%s] must be declared abstract or referenced as a dependency', + 'Extended class [%s] must be declared abstract or instantiated', $classNode->className ), file: $classNode->file, diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index 62ba48cb..a40a36ec 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -469,7 +469,7 @@ public function testYagniRulesIgnoreSelfReferences(): void $this->assertSame('App\UnusedTrait', $traitViolations[0]->className); } - public function testExtendedClassMustBeAbstractOrReferencedRuleFlagsOnlyUnreferencedParent(): void + public function testExtendedClassMustBeAbstractOrInstantiatedRuleFlagsUninstantiatedParent(): void { $basePath = $this->makeTempProject([ 'src/BaseRepository.php' => 'analyse($architecture, [], null, AnalyserOptions::sequential()) - ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); // BaseRepository is only ever used as a parent class; extending it // does not count as a reference, so it should be abstract. @@ -490,7 +490,33 @@ public function testExtendedClassMustBeAbstractOrReferencedRuleFlagsOnlyUnrefere $this->assertSame('App\BaseRepository', $violations[0]->className); } - public function testExtendedClassMustBeAbstractOrReferencedRulePassesWhenParentIsInstantiated(): void + public function testExtendedClassMustBeAbstractOrInstantiatedRuleFlagsTypeHintedButUninstantiatedParent(): void + { + $consumer = 'makeTempProject([ + 'src/BaseRepository.php' => ' ' $consumer, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + // Type hints, instanceof, and ::class keep working on an abstract + // class — only instantiation requires it to stay concrete. + $this->assertCount(1, $violations); + $this->assertSame('App\BaseRepository', $violations[0]->className); + } + + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesWhenParentIsInstantiated(): void { $factory = 'analyse($architecture, [], null, AnalyserOptions::sequential()) - ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); $this->assertCount(0, $violations); } - public function testExtendedClassMustBeAbstractOrReferencedRulePassesWhenChildInstantiatesParent(): void + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesWhenChildInstantiatesParent(): void { // The extends clause itself must not count as a reference, but the // child's `new BaseRepository()` must: making the parent abstract @@ -532,12 +558,38 @@ public function testExtendedClassMustBeAbstractOrReferencedRulePassesWhenChildIn $violations = (new Analyser($basePath)) ->analyse($architecture, [], null, AnalyserOptions::sequential()) - ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + $this->assertCount(0, $violations); + } + + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnDynamicInstantiation(): void + { + // The constant-expression evaluator resolves `$class = X::class; + // new $class()` — dynamic instantiation still keeps the parent + // concrete. + $factory = 'makeTempProject([ + 'src/BaseRepository.php' => ' ' $factory, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); $this->assertCount(0, $violations); } - public function testExtendedClassMustBeAbstractOrReferencedRulePassesOnSelfAndParentInstantiation(): void + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnSelfAndParentInstantiation(): void { // `new self()` resolves to the class itself even when called through a // subclass, and `new parent()` resolves to the extended class — both @@ -562,7 +614,7 @@ public function testExtendedClassMustBeAbstractOrReferencedRulePassesOnSelfAndPa $violations = (new Analyser($basePath)) ->analyse($architecture, [], null, AnalyserOptions::sequential()) - ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED); + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); // Connection is protected by both `new self()` and `new parent()`; // ConnectionPool is extended but never referenced, so only it is @@ -651,10 +703,13 @@ public function testYagniRulesDoNotFlagAbstractionsReferencedByTopLevelAnonymous public function testYagniRulesRecognizeProceduralReferencesOnCachedRun(): void { $functions = 'makeTempProject([ 'src/Contract.php' => ' ' ' $functions, ]); $analysisResultCache = new AnalysisResultCache($basePath, new FileHashProvider(), 'cache'); @@ -675,10 +730,13 @@ public function testYagniRulesRecognizeProceduralReferencesOnCachedRun(): void public function testYagniRulesRecognizeProceduralReferencesOnCachedRunWithFileAnalysis(): void { $functions = 'makeTempProject([ 'src/Contract.php' => ' ' ' $functions, ]); $analysisResultCache = new AnalysisResultCache($basePath, new FileHashProvider(), 'cache'); @@ -785,8 +843,12 @@ public function testYagniRulesRecognizeUsageWithParallelRunner(): void 'src/UsedBase.php' => ' ' ' ' ' $consumer, - 'src/functions.php' => ' 'assertSame([], $classCollector->getFileReferences()); } - public function testCollectsInstantiationsAsFileReferences(): void + public function testCollectsInstantiations(): void { $code = 'assertSame( ['/fake/path/Foo.php' => ['App\Service']], - $classCollector->getFileReferences() + $classCollector->getFileInstantiations() ); + $this->assertSame([], $classCollector->getFileReferences()); } public function testResolvesSelfStaticAndParentInstantiations(): void @@ -140,10 +141,69 @@ public function testResolvesSelfStaticAndParentInstantiations(): void $this->assertSame( ['/fake/path/Foo.php' => ['App\Repository', 'App\BaseRepository']], - $classCollector->getFileReferences() + $classCollector->getFileInstantiations() + ); + } + + public function testResolvesVariableClassNameInstantiations(): void + { + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Base', 'App\StringBase', 'App\Concatenated', 'App\Joined']], + $classCollector->getFileInstantiations() + ); + } + + public function testResolvesSelfClassConstantInstantiation(): void + { + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Registry']], + $classCollector->getFileInstantiations() ); } + public function testDropsVariableClassNameOnUnresolvableReassignment(): void + { + $code = 'makeCollector($code); + + // The reassignment cannot be evaluated statically, so the earlier + // constant value must not leak into the instantiation. + $this->assertSame([], $classCollector->getFileInstantiations()); + } + + public function testIgnoresUnresolvableClassNameExpressions(): void + { + $code = 'kept = \'App\\Prop\'; return new $class(); } }'; + + $classCollector = $this->makeCollector($code); + + // Runtime-dependent concatenation, non-class-shaped strings, and + // property assignments resolve to nothing. + $this->assertSame([], $classCollector->getFileInstantiations()); + } + public function testDoesNotCollectDynamicOrAnonymousInstantiations(): void { $code = 'assertSame([], $classCollector->getFileReferences()); + $this->assertSame([], $classCollector->getFileInstantiations()); } public function testIgnoresRelativeInstantiationOutsideClassScope(): void @@ -163,7 +223,7 @@ public function testIgnoresRelativeInstantiationOutsideClassScope(): void // PHP itself rejects it at runtime. $classCollector = $this->makeCollector('assertSame([], $classCollector->getFileReferences()); + $this->assertSame([], $classCollector->getFileInstantiations()); } public function testCollectsFinalClass(): void diff --git a/tests/Analyser/ClassNodeTest.php b/tests/Analyser/ClassNodeTest.php index b8892f6f..992b7f33 100644 --- a/tests/Analyser/ClassNodeTest.php +++ b/tests/Analyser/ClassNodeTest.php @@ -292,7 +292,7 @@ className: 'App\\Domain\\OrderRepositoryInterface', $this->assertFalse($classNode->isImplemented); } - public function testSetUsedTogglesIsUsedFlag(): void + public function testSetReferencedTogglesIsReferencedFlag(): void { $classNode = new ClassNode( className: 'App\\Domain\\TimestampableTrait', @@ -316,6 +316,29 @@ className: 'App\\Domain\\TimestampableTrait', $this->assertFalse($classNode->isReferenced); } + public function testSetInstantiatedTogglesIsInstantiatedFlag(): void + { + $classNode = new ClassNode( + className: 'App\\Domain\\BaseRepository', + file: '/src/BaseRepository.php', + line: 5, + layer: 'Domain', + extends: null, + isAbstract: false, + isFinal: false, + isInterface: false, + isReadonly: false, + ); + + $this->assertFalse($classNode->isInstantiated); + + $classNode->setInstantiated(true); + $this->assertTrue($classNode->isInstantiated); + + $classNode->setInstantiated(false); + $this->assertFalse($classNode->isInstantiated); + } + public function testDependsOnMatchesExistingClassesExactly(): void { $classNode = new ClassNode( diff --git a/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php b/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php index e8ed6de0..7821c64b 100644 --- a/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php +++ b/tests/Analyser/Parallel/ParallelClassNodeExtractorTest.php @@ -490,6 +490,45 @@ public static function invalidFileReferencesEntryProvider(): Iterator yield 'entry with non-string reference' => [['Foo.php' => [1]]]; } + /** + * @return Iterator + */ + public static function invalidFileInstantiationsProvider(): Iterator + { + yield 'not an array' => ['invalid']; + yield 'entry not an array' => [['Foo.php' => 'invalid']]; + yield 'entry with non-string instantiation' => [['Foo.php' => [1]]]; + } + + #[DataProvider('invalidFileInstantiationsProvider')] + public function testExtractThrowsWhenFileInstantiationsPayloadIsInvalid(mixed $invalidFileInstantiations): void + { + $GLOBALS['mock_file_get_contents_payload'] = [ + 'nodes' => [], + 'fileAnalyses' => [], + 'anonymousClassNodes' => [], + 'fileReferences' => [], + 'fileInstantiations' => $invalidFileInstantiations, + 'error' => null, + ]; + + $dir = $this->makeTemporaryDirectory('structarmed-parallel-test'); + $file = $dir . '/Foo.php'; + file_put_contents($file, 'expectException(RuntimeException::class); + $this->expectExceptionMessage('Parallel analysis worker returned invalid file instantiations.'); + + try { + $parallelClassNodeExtractor->extract([$file]); + } finally { + $GLOBALS['mock_file_get_contents_payload'] = null; + $GLOBALS['mock_tracked_tempnam_files'] = []; + } + } + #[DataProvider('invalidFileReferencesEntryProvider')] public function testExtractThrowsWhenFileReferencesEntryIsInvalid(mixed $invalidFileReferences): void { diff --git a/tests/Cache/AnalysisResultCacheTest.php b/tests/Cache/AnalysisResultCacheTest.php index 221c08d6..c784802d 100644 --- a/tests/Cache/AnalysisResultCacheTest.php +++ b/tests/Cache/AnalysisResultCacheTest.php @@ -621,6 +621,7 @@ traits: ['App\Helper'], null, $anonymousClassNodes, ['App\ReferencedInFunction'], + ['App\InstantiatedInFunction'], ); $loaded = $analysisResultCache->loadClassNodes($sourceFile, 'config'); @@ -629,6 +630,7 @@ traits: ['App\Helper'], $this->assertEquals($classNodes, $loaded['classNodes']); $this->assertEquals($anonymousClassNodes, $loaded['anonymousClassNodes']); $this->assertSame(['App\ReferencedInFunction'], $loaded['fileReferences']); + $this->assertSame(['App\InstantiatedInFunction'], $loaded['fileInstantiations']); } finally { if (file_exists($sourceFile)) { unlink($sourceFile); @@ -663,6 +665,7 @@ public function testClassNodesLoadOldCachePayloadWithoutAnonymousClassNodes(): v $this->assertEquals($classNodes, $loaded['classNodes']); $this->assertSame([], $loaded['anonymousClassNodes']); $this->assertSame([], $loaded['fileReferences']); + $this->assertSame([], $loaded['fileInstantiations']); } finally { if (file_exists($sourceFile)) { unlink($sourceFile); @@ -715,6 +718,33 @@ public function testLoadClassNodesRejectsCorruptedFileReferencesPayload(): void } } + public function testLoadClassNodesRejectsCorruptedFileInstantiationsPayload(): void + { + $cacheDirectory = $this->createTempDirectory(); + $sourceFile = $cacheDirectory . '/Foo.php'; + $analysisResultCache = new AnalysisResultCache(__DIR__, new FileHashProvider(), $cacheDirectory); + + file_put_contents($sourceFile, 'storeClassNodes($sourceFile, 'config', [$this->makeClassNode($sourceFile)]); + + $cacheFile = $this->firstJsonFile($cacheDirectory); + $payload = json_decode((string) file_get_contents($cacheFile), true); + $this->assertIsArray($payload); + $payload['fileInstantiations'] = ['App\Base', 1]; + file_put_contents($cacheFile, json_encode($payload, JSON_THROW_ON_ERROR)); + + $this->assertNull($analysisResultCache->loadClassNodes($sourceFile, 'config')); + } finally { + if (file_exists($sourceFile)) { + unlink($sourceFile); + } + + $this->removeTempDirectory($cacheDirectory); + } + } + #[DataProvider('corruptedAnonymousClassNodesProvider')] public function testLoadClassNodesRejectsCorruptedAnonymousClassNodesPayload(mixed $corrupted): void { diff --git a/tests/Preset/PresetTest.php b/tests/Preset/PresetTest.php index 1acf676a..067698f2 100644 --- a/tests/Preset/PresetTest.php +++ b/tests/Preset/PresetTest.php @@ -14,7 +14,7 @@ use Boundwize\StructArmed\Preset\Presets\Psr4Preset; use Boundwize\StructArmed\Preset\Presets\ResolvesSourceLayerNameTrait; use Boundwize\StructArmed\Preset\Presets\YagniPreset; -use Boundwize\StructArmed\Rule\Rules\Class_\ExtendedClassMustBeAbstractOrReferencedRule; +use Boundwize\StructArmed\Rule\Rules\Class_\ExtendedClassMustBeAbstractOrInstantiatedRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedInterfaceRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedTraitRule; @@ -56,8 +56,8 @@ public function testYagniPresetRegistersSourceLayerAndRules(): void $rules[YagniPreset::TRAIT_MUST_BE_USED] ?? null ); $this->assertInstanceOf( - ExtendedClassMustBeAbstractOrReferencedRule::class, - $rules[YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_REFERENCED] ?? null + ExtendedClassMustBeAbstractOrInstantiatedRule::class, + $rules[YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED] ?? null ); } diff --git a/tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleFixTest.php b/tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleFixTest.php new file mode 100644 index 00000000..6c7e6ccb --- /dev/null +++ b/tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleFixTest.php @@ -0,0 +1,51 @@ +makeTemporaryDirectory('structarmed-yagni-extended'); + $file = $temporaryDirectory . '/SomeBaseClass.php'; + + file_put_contents( + $file, + "assertTrue($extendedClassMustBeAbstractOrInstantiatedRule->fix(new RuleViolation( + message: 'Extended class [App\\SomeBaseClass] must be declared abstract' + . ' or referenced as a dependency', + file: $file, + line: 7, + className: 'App\\SomeBaseClass', + layer: 'Domain', + ))); + $this->assertFileExists($file); + $this->assertStringContainsString( + 'abstract class SomeBaseClass', + (string) file_get_contents($file) + ); + } +} diff --git a/tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleTest.php b/tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleTest.php new file mode 100644 index 00000000..a13f5e13 --- /dev/null +++ b/tests/Rule/Class_/ExtendedClassMustBeAbstractOrInstantiatedRuleTest.php @@ -0,0 +1,225 @@ +makeNode(isExtended: true); + + $violation = $extendedClassMustBeAbstractOrInstantiatedRule->evaluate($classNode); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertStringContainsString('must be declared abstract', $violation->message); + } + + public function testPassesWhenClassIsNotExtended(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isExtended: false); + + $this->assertNotInstanceOf( + RuleViolation::class, + $extendedClassMustBeAbstractOrInstantiatedRule->evaluate($classNode) + ); + } + + public function testPassesWhenExtendedClassIsInstantiated(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isExtended: true, isInstantiated: true); + + $this->assertNotInstanceOf( + RuleViolation::class, + $extendedClassMustBeAbstractOrInstantiatedRule->evaluate($classNode) + ); + } + + public function testViolatesWhenExtendedClassIsOnlyReferencedButNotInstantiated(): void + { + // Type hints, instanceof, and ::class keep working on an abstract + // class, so a plain reference is not enough to keep it concrete. + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isExtended: true, isReferenced: true); + + $this->assertInstanceOf( + RuleViolation::class, + $extendedClassMustBeAbstractOrInstantiatedRule->evaluate($classNode) + ); + } + + public function testIsExtendedClassAware(): void + { + $this->assertInstanceOf( + ExtendedClassAwareRuleInterface::class, + new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Domain') + ); + } + + public function testIsFixable(): void + { + $this->assertInstanceOf( + FixableInterface::class, + new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Domain') + ); + } + + public function testCreatesAddAbstractClassFixerVisitor(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $reflectionMethod = new ReflectionMethod( + $extendedClassMustBeAbstractOrInstantiatedRule, + 'createFixerVisitor' + ); + $addAbstractClassVisitor = $reflectionMethod->invoke( + $extendedClassMustBeAbstractOrInstantiatedRule, + new RuleViolation( + message: 'Extended class [App\\BaseRepository] must be declared abstract' + . ' or referenced as a dependency', + file: '/src/BaseRepository.php', + line: 1, + className: 'App\\BaseRepository', + layer: 'Domain', + ) + ); + + $this->assertInstanceOf(AddAbstractClassVisitor::class, $addAbstractClassVisitor); + } + + public function testDoesNotApplyToWrongLayer(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(layer: 'Infrastructure'); + + $this->assertFalse($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToAbstractClasses(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isAbstract: true); + + $this->assertFalse($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToInterfaces(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isInterface: true); + + $this->assertFalse($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToTraits(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isTrait: true); + + $this->assertFalse($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToEnums(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(isEnum: true); + + $this->assertFalse($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } + + public function testAppliesToMatchingPattern(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain', + classNamePattern: '/Repository$/' + ); + $classNode = $this->makeNode(); + + $this->assertTrue($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } + + public function testDoesNotApplyToNonMatchingPattern(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain', + classNamePattern: '/Service$/' + ); + $classNode = $this->makeNode(); + + $this->assertFalse($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } + + public function testAppliesToLayerWhenNoPatternConfigured(): void + { + $extendedClassMustBeAbstractOrInstantiatedRule = new ExtendedClassMustBeAbstractOrInstantiatedRule( + layer: 'Domain' + ); + $classNode = $this->makeNode(); + + $this->assertTrue($extendedClassMustBeAbstractOrInstantiatedRule->appliesTo($classNode)); + } +} diff --git a/tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php b/tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php deleted file mode 100644 index 57e1ce00..00000000 --- a/tests/Rule/Class_/ExtendedClassMustBeAbstractOrReferencedRuleTest.php +++ /dev/null @@ -1,243 +0,0 @@ -makeNode(isExtended: true); - - $violation = $extendedClassMustBeAbstractOrReferencedRule->evaluate($classNode); - - $this->assertInstanceOf(RuleViolation::class, $violation); - $this->assertStringContainsString('must be declared abstract', $violation->message); - } - - public function testPassesWhenClassIsNotExtended(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(isExtended: false); - - $this->assertNotInstanceOf( - RuleViolation::class, - $extendedClassMustBeAbstractOrReferencedRule->evaluate($classNode) - ); - } - - public function testPassesWhenExtendedClassIsReferenced(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(isExtended: true, isReferenced: true); - - $this->assertNotInstanceOf( - RuleViolation::class, - $extendedClassMustBeAbstractOrReferencedRule->evaluate($classNode) - ); - } - - public function testIsExtendedClassAware(): void - { - $this->assertInstanceOf( - ExtendedClassAwareRuleInterface::class, - new ExtendedClassMustBeAbstractOrReferencedRule(layer: 'Domain') - ); - } - - public function testIsFixable(): void - { - $this->assertInstanceOf( - FixableInterface::class, - new ExtendedClassMustBeAbstractOrReferencedRule(layer: 'Domain') - ); - } - - public function testCreatesAddAbstractClassFixerVisitor(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $reflectionMethod = new ReflectionMethod( - $extendedClassMustBeAbstractOrReferencedRule, - 'createFixerVisitor' - ); - $addAbstractClassVisitor = $reflectionMethod->invoke( - $extendedClassMustBeAbstractOrReferencedRule, - new RuleViolation( - message: 'Extended class [App\\BaseRepository] must be declared abstract' - . ' or referenced as a dependency', - file: '/src/BaseRepository.php', - line: 1, - className: 'App\\BaseRepository', - layer: 'Domain', - ) - ); - - $this->assertInstanceOf(AddAbstractClassVisitor::class, $addAbstractClassVisitor); - } - - public function testDoesNotApplyToWrongLayer(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(layer: 'Infrastructure'); - - $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testDoesNotApplyToAbstractClasses(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(isAbstract: true); - - $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testDoesNotApplyToInterfaces(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(isInterface: true); - - $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testDoesNotApplyToTraits(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(isTrait: true); - - $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testDoesNotApplyToEnums(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(isEnum: true); - - $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testAppliesToMatchingPattern(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain', - classNamePattern: '/Repository$/' - ); - $classNode = $this->makeNode(); - - $this->assertTrue($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testDoesNotApplyToNonMatchingPattern(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain', - classNamePattern: '/Service$/' - ); - $classNode = $this->makeNode(); - - $this->assertFalse($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testAppliesToLayerWhenNoPatternConfigured(): void - { - $extendedClassMustBeAbstractOrReferencedRule = new ExtendedClassMustBeAbstractOrReferencedRule( - layer: 'Domain' - ); - $classNode = $this->makeNode(); - - $this->assertTrue($extendedClassMustBeAbstractOrReferencedRule->appliesTo($classNode)); - } - - public function testFixDeclaresClassAbstract(): void - { - $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-extended'); - $file = $temporaryDirectory . '/SomeBaseClass.php'; - - file_put_contents( - $file, - "assertTrue($extendedClassMustBeAbstractOrReferencedRule->fix(new RuleViolation( - message: 'Extended class [App\\SomeBaseClass] must be declared abstract' - . ' or referenced as a dependency', - file: $file, - line: 7, - className: 'App\\SomeBaseClass', - layer: 'Domain', - ))); - $this->assertFileExists($file); - $this->assertStringContainsString( - 'abstract class SomeBaseClass', - (string) file_get_contents($file) - ); - } -} diff --git a/tests/Rule/Class_/MustBeUsedAbstractClassRuleFixTest.php b/tests/Rule/Class_/MustBeUsedAbstractClassRuleFixTest.php new file mode 100644 index 00000000..b3dd1d05 --- /dev/null +++ b/tests/Rule/Class_/MustBeUsedAbstractClassRuleFixTest.php @@ -0,0 +1,43 @@ +makeTemporaryDirectory('structarmed-yagni-abstract'); + $file = $temporaryDirectory . '/AbstractHandler.php'; + + file_put_contents( + $file, + "assertTrue($mustBeUsedAbstractClassRule->fix(new RuleViolation( + message: 'Abstract class [App\\AbstractHandler] must be extended by a class', + file: $file, + line: 7, + className: 'App\\AbstractHandler', + layer: 'Domain', + ))); + $this->assertFileDoesNotExist($file); + } +} diff --git a/tests/Rule/Class_/MustBeUsedAbstractClassRuleTest.php b/tests/Rule/Class_/MustBeUsedAbstractClassRuleTest.php index 1aa7e833..0bed47aa 100644 --- a/tests/Rule/Class_/MustBeUsedAbstractClassRuleTest.php +++ b/tests/Rule/Class_/MustBeUsedAbstractClassRuleTest.php @@ -10,19 +10,14 @@ use Boundwize\StructArmed\Rule\Fixer\PhpParser\ClassLike\RemoveClassLikeVisitor; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; use Boundwize\StructArmed\Rule\RuleViolation; -use Boundwize\StructArmed\Tests\Support\TemporaryDirectoryCleanupTrait; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use ReflectionMethod; -use function file_put_contents; - #[CoversClass(MustBeUsedAbstractClassRule::class)] #[CoversClass(RemoveClassLikeVisitor::class)] final class MustBeUsedAbstractClassRuleTest extends TestCase { - use TemporaryDirectoryCleanupTrait; - private function makeNode( string $className = 'App\\Domain\\AbstractHandler', string $layer = 'Domain', @@ -178,26 +173,4 @@ classNamePattern: '/Base$/' $this->assertFalse($mustBeUsedAbstractClassRule->appliesTo($classNode)); } - - public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void - { - $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-abstract'); - $file = $temporaryDirectory . '/AbstractHandler.php'; - - file_put_contents( - $file, - "assertTrue($mustBeUsedAbstractClassRule->fix(new RuleViolation( - message: 'Abstract class [App\\AbstractHandler] must be extended by a class', - file: $file, - line: 7, - className: 'App\\AbstractHandler', - layer: 'Domain', - ))); - $this->assertFileDoesNotExist($file); - } } diff --git a/tests/Rule/Class_/MustBeUsedInterfaceRuleFixTest.php b/tests/Rule/Class_/MustBeUsedInterfaceRuleFixTest.php new file mode 100644 index 00000000..152db619 --- /dev/null +++ b/tests/Rule/Class_/MustBeUsedInterfaceRuleFixTest.php @@ -0,0 +1,109 @@ +makeTemporaryDirectory('structarmed-yagni-interface'); + $file = $temporaryDirectory . '/UnusedInterface.php'; + + file_put_contents( + $file, + "assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( + message: 'Interface [App\\UnusedInterface] must be implemented by a class' + . ' or extended by another interface', + file: $file, + line: 7, + className: 'App\\UnusedInterface', + layer: 'Domain', + ))); + $this->assertFileDoesNotExist($file); + } + + public function testFixKeepsFileWhenDeclareBlockContainsExecutableCode(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); + $file = $temporaryDirectory . '/ticks.php'; + + // The block form `declare(ticks=1) { ... }` carries executable + // statements — removing the interface must not delete the file. + file_put_contents( + $file, + "assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( + message: 'Interface [UnusedInterface] must be implemented by a class' + . ' or extended by another interface', + file: $file, + line: 7, + className: 'UnusedInterface', + layer: 'Domain', + ))); + $this->assertFileExists($file); + + $fixedCode = (string) file_get_contents($file); + + $this->assertStringNotContainsString('interface UnusedInterface', $fixedCode); + $this->assertStringContainsString("echo 'KEEP ME';", $fixedCode); + } + + public function testFixKeepsFileWhenOtherCodeRemains(): void + { + $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); + $file = $temporaryDirectory . '/Contracts.php'; + + file_put_contents( + $file, + "assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( + message: 'Interface [App\\UnusedInterface] must be implemented by a class' + . ' or extended by another interface', + file: $file, + line: 7, + className: 'App\\UnusedInterface', + layer: 'Domain', + ))); + $this->assertFileExists($file); + + $fixedCode = (string) file_get_contents($file); + + $this->assertStringNotContainsString('interface UnusedInterface', $fixedCode); + $this->assertStringContainsString('final class Order', $fixedCode); + } +} diff --git a/tests/Rule/Class_/MustBeUsedInterfaceRuleTest.php b/tests/Rule/Class_/MustBeUsedInterfaceRuleTest.php index 471408d1..4506bf86 100644 --- a/tests/Rule/Class_/MustBeUsedInterfaceRuleTest.php +++ b/tests/Rule/Class_/MustBeUsedInterfaceRuleTest.php @@ -6,28 +6,18 @@ use Boundwize\StructArmed\Analyser\ClassNode; use Boundwize\StructArmed\Rule\FixableInterface; -use Boundwize\StructArmed\Rule\Fixer\PhpParser\AbstractPhpParserFixableRule; use Boundwize\StructArmed\Rule\Fixer\PhpParser\ClassLike\RemoveClassLikeVisitor; -use Boundwize\StructArmed\Rule\Fixer\PhpParser\PhpParserFixerProcessor; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedInterfaceRule; use Boundwize\StructArmed\Rule\RuleViolation; use Boundwize\StructArmed\Rule\UsedInterfaceAwareRuleInterface; -use Boundwize\StructArmed\Tests\Support\TemporaryDirectoryCleanupTrait; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use ReflectionMethod; -use function file_get_contents; -use function file_put_contents; - #[CoversClass(MustBeUsedInterfaceRule::class)] #[CoversClass(RemoveClassLikeVisitor::class)] -#[CoversClass(AbstractPhpParserFixableRule::class)] -#[CoversClass(PhpParserFixerProcessor::class)] final class MustBeUsedInterfaceRuleTest extends TestCase { - use TemporaryDirectoryCleanupTrait; - private function makeNode( string $className = 'App\\Domain\\OrderRepositoryInterface', string $layer = 'Domain', @@ -171,87 +161,4 @@ classNamePattern: '/Repository$/' $this->assertFalse($mustBeUsedInterfaceRule->appliesTo($classNode)); } - - public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void - { - $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); - $file = $temporaryDirectory . '/UnusedInterface.php'; - - file_put_contents( - $file, - "assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( - message: 'Interface [App\\UnusedInterface] must be implemented by a class' - . ' or extended by another interface', - file: $file, - line: 7, - className: 'App\\UnusedInterface', - layer: 'Domain', - ))); - $this->assertFileDoesNotExist($file); - } - - public function testFixKeepsFileWhenDeclareBlockContainsExecutableCode(): void - { - $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); - $file = $temporaryDirectory . '/ticks.php'; - - // The block form `declare(ticks=1) { ... }` carries executable - // statements — removing the interface must not delete the file. - file_put_contents( - $file, - "assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( - message: 'Interface [UnusedInterface] must be implemented by a class' - . ' or extended by another interface', - file: $file, - line: 7, - className: 'UnusedInterface', - layer: 'Domain', - ))); - $this->assertFileExists($file); - - $fixedCode = (string) file_get_contents($file); - - $this->assertStringNotContainsString('interface UnusedInterface', $fixedCode); - $this->assertStringContainsString("echo 'KEEP ME';", $fixedCode); - } - - public function testFixKeepsFileWhenOtherCodeRemains(): void - { - $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-interface'); - $file = $temporaryDirectory . '/Contracts.php'; - - file_put_contents( - $file, - "assertTrue($mustBeUsedInterfaceRule->fix(new RuleViolation( - message: 'Interface [App\\UnusedInterface] must be implemented by a class' - . ' or extended by another interface', - file: $file, - line: 7, - className: 'App\\UnusedInterface', - layer: 'Domain', - ))); - $this->assertFileExists($file); - - $fixedCode = (string) file_get_contents($file); - - $this->assertStringNotContainsString('interface UnusedInterface', $fixedCode); - $this->assertStringContainsString('final class Order', $fixedCode); - } } diff --git a/tests/Rule/Class_/MustBeUsedTraitRuleFixTest.php b/tests/Rule/Class_/MustBeUsedTraitRuleFixTest.php new file mode 100644 index 00000000..bf1d0b1e --- /dev/null +++ b/tests/Rule/Class_/MustBeUsedTraitRuleFixTest.php @@ -0,0 +1,43 @@ +makeTemporaryDirectory('structarmed-yagni-trait'); + $file = $temporaryDirectory . '/UnusedTrait.php'; + + file_put_contents( + $file, + "assertTrue($mustBeUsedTraitRule->fix(new RuleViolation( + message: 'Trait [App\\UnusedTrait] must be used by a class, trait, or enum', + file: $file, + line: 7, + className: 'App\\UnusedTrait', + layer: 'Domain', + ))); + $this->assertFileDoesNotExist($file); + } +} diff --git a/tests/Rule/Class_/MustBeUsedTraitRuleTest.php b/tests/Rule/Class_/MustBeUsedTraitRuleTest.php index 29b74332..16b789af 100644 --- a/tests/Rule/Class_/MustBeUsedTraitRuleTest.php +++ b/tests/Rule/Class_/MustBeUsedTraitRuleTest.php @@ -10,19 +10,14 @@ use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedTraitRule; use Boundwize\StructArmed\Rule\RuleViolation; use Boundwize\StructArmed\Rule\UsedTraitAwareRuleInterface; -use Boundwize\StructArmed\Tests\Support\TemporaryDirectoryCleanupTrait; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use ReflectionMethod; -use function file_put_contents; - #[CoversClass(MustBeUsedTraitRule::class)] #[CoversClass(RemoveClassLikeVisitor::class)] final class MustBeUsedTraitRuleTest extends TestCase { - use TemporaryDirectoryCleanupTrait; - private function makeNode( string $className = 'App\\Domain\\TimestampableTrait', string $layer = 'Domain', @@ -144,26 +139,4 @@ public function testDoesNotApplyToNonMatchingPattern(): void $this->assertFalse($mustBeUsedTraitRule->appliesTo($classNode)); } - - public function testFixDeletesFileWhenOnlyBoilerplateRemains(): void - { - $temporaryDirectory = $this->makeTemporaryDirectory('structarmed-yagni-trait'); - $file = $temporaryDirectory . '/UnusedTrait.php'; - - file_put_contents( - $file, - "assertTrue($mustBeUsedTraitRule->fix(new RuleViolation( - message: 'Trait [App\\UnusedTrait] must be used by a class, trait, or enum', - file: $file, - line: 7, - className: 'App\\UnusedTrait', - layer: 'Domain', - ))); - $this->assertFileDoesNotExist($file); - } } From 5308acc2e6c83d0124481683ae384dbab9fa2729 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 19 Aug 2026 23:50:15 +0700 Subject: [PATCH 13/19] handle variable classnames collection --- src/Analyser/ClassCollector.php | 38 +++++++++++++++++---------- tests/Analyser/AnalyserTest.php | 28 ++++++++++++++++++++ tests/Analyser/ClassCollectorTest.php | 27 ++++++++++++++++--- 3 files changed, 75 insertions(+), 18 deletions(-) diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index 3e78175a..b60afb78 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -59,6 +59,7 @@ use PhpParser\Node\Stmt\While_; use PhpParser\NodeVisitorAbstract; +use function array_keys; use function array_pop; use function array_unique; use function array_values; @@ -117,10 +118,11 @@ final class ClassCollector extends NodeVisitorAbstract private array $currentFileInstantiations = []; /** - * Variables currently known to hold a constant class-name string, so - * `new $variable` instantiations can be resolved. + * Constant class-name strings each variable may hold (a union across all + * of its resolvable assignments), so `new $variable` instantiations can be + * resolved to every possible target. * - * @var array + * @var array> */ private array $variableClassNames = []; @@ -581,11 +583,19 @@ private function collectInstantiation(New_ $new): void return; } - // `new $class` where the variable holds a constant class-name value, - // or `new (X::class)` / `new ('App\X')` class expressions. - $className = $class instanceof Variable && is_string($class->name) - ? ($this->variableClassNames[$class->name] ?? null) - : $this->resolveClassNameExpr($class); + // `new $class` instantiates any of the constant class-name values the + // variable may hold — conditional reassignments make every recorded + // possibility reachable at runtime. + if ($class instanceof Variable && is_string($class->name)) { + foreach (array_keys($this->variableClassNames[$class->name] ?? []) as $possibleClassName) { + $this->currentFileInstantiations[] = $possibleClassName; + } + + return; + } + + // `new (X::class)` / `new ('App\X')` class expressions. + $className = $this->resolveClassNameExpr($class); if ($className !== null) { $this->currentFileInstantiations[] = $className; @@ -623,8 +633,11 @@ private function resolveClassLikeName(Name $name): ?string /** * Track `$variable = ` assignments so a - * later `new $variable` can be resolved. Over-approximation is safe: a - * recorded instantiation only keeps a class concrete or alive. + * later `new $variable` can be resolved. Every resolvable value is kept as + * a possibility — a conditional reassignment does not replace the earlier + * one, since either branch may run. Over-approximation is the safe + * direction: a recorded instantiation only keeps a class concrete or + * alive, while a missed one could let the fixer break runtime code. */ private function trackVariableClassName(Assign $assign): void { @@ -635,13 +648,10 @@ private function trackVariableClassName(Assign $assign): void $className = $this->resolveClassNameExpr($assign->expr); if ($className === null) { - // Reassigned to something unresolvable — drop the stale name. - unset($this->variableClassNames[$assign->var->name]); - return; } - $this->variableClassNames[$assign->var->name] = $className; + $this->variableClassNames[$assign->var->name][$className] = true; } /** diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index a40a36ec..0d35e8a0 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -589,6 +589,34 @@ public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnDynamic $this->assertCount(0, $violations); } + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnConditionalDynamicInstantiation(): void + { + // `create(false)` instantiates Base at runtime even though the + // traversal sees Child assigned last — every possible value of the + // variable must keep its class concrete. + $factory = 'makeTempProject([ + 'src/Base.php' => ' ' $factory, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + $this->assertCount(0, $violations); + } + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnSelfAndParentInstantiation(): void { // `new self()` resolves to the class itself even when called through a diff --git a/tests/Analyser/ClassCollectorTest.php b/tests/Analyser/ClassCollectorTest.php index 09ec482c..26f0f5ca 100644 --- a/tests/Analyser/ClassCollectorTest.php +++ b/tests/Analyser/ClassCollectorTest.php @@ -177,7 +177,22 @@ public function testResolvesSelfClassConstantInstantiation(): void ); } - public function testDropsVariableClassNameOnUnresolvableReassignment(): void + public function testCollectsAllPossibleClassNamesOnConditionalReassignment(): void + { + // Either branch may run at runtime, so both classes are instantiable. + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Base', 'App\Child']], + $classCollector->getFileInstantiations() + ); + } + + public function testKeepsEarlierClassNameOnUnresolvableReassignment(): void { $code = 'makeCollector($code); - // The reassignment cannot be evaluated statically, so the earlier - // constant value must not leak into the instantiation. - $this->assertSame([], $classCollector->getFileInstantiations()); + // The reassignment cannot be evaluated statically, but the earlier + // constant value may still reach the instantiation — keeping it is + // the safe over-approximation. + $this->assertSame( + ['/fake/path/Foo.php' => ['App\Base']], + $classCollector->getFileInstantiations() + ); } public function testIgnoresUnresolvableClassNameExpressions(): void From f939dd05118556eefaa8efbdab2b955cbbc57065 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 20 Aug 2026 00:00:23 +0700 Subject: [PATCH 14/19] add more scenario check --- docs/available-rules.md | 2 +- src/Analyser/Analyser.php | 34 ++++++++++++++-- src/Analyser/ClassCollector.php | 58 +++++++++++++++++++++------ tests/Analyser/AnalyserTest.php | 51 +++++++++++++++++++++++ tests/Analyser/ClassCollectorTest.php | 51 ++++++++++++++++++----- 5 files changed, 168 insertions(+), 28 deletions(-) diff --git a/docs/available-rules.md b/docs/available-rules.md index ac46e426..215b8ac8 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -78,7 +78,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `ClassNameMustBeStudlyCapsRule` | `new ClassNameMustBeStudlyCapsRule(layer: 'Source')` | Class names use StudlyCaps. | | `ClassNameMustHaveSuffixRule` | `new ClassNameMustHaveSuffixRule(layer: 'Controller', suffix: 'Controller')` | Classes in a layer have the required suffix. | | `ClassNameMustNotHavePrefixRule` | `new ClassNameMustNotHavePrefixRule(layer: 'Model', prefix: 'Model')` | Classes in a layer do not use a forbidden prefix. | -| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count. Supports `--fix` by adding the `abstract` modifier. | +| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count — but when the scanned code contains a dynamic `new` whose target cannot be resolved (e.g. a factory's `new $class` fed by call sites), referenced classes conservatively count as instantiated. Supports `--fix` by adding the `abstract` modifier. | | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 71831ff4..66403599 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -762,12 +762,29 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e $used[strtolower($trait)] = true; } - $selfKey = strtolower($classNode->className); + // A node's own inheritance-clause names (and the imports that + // exist for them) are structural relations, not value references. + // Excluding them keeps "referenced" meaningful for the unresolved + // dynamic instantiation check below: a class extended by a child + // is not thereby a possible `new $class` target. The usage-aware + // deletion rules are unaffected — each combines this flag with its + // structural extended/implemented/trait marking. + $excludedKeys = [strtolower($classNode->className) => true]; + + if ($classNode->extends !== null) { + $excludedKeys[strtolower($classNode->extends)] = true; + } + + foreach ([$classNode->implements, $classNode->interfaceExtends, $classNode->traits] as $clauseNames) { + foreach ($clauseNames as $clauseName) { + $excludedKeys[strtolower($clauseName)] = true; + } + } foreach ($classNode->dependencies as $dependency) { $dependencyKey = strtolower($dependency); - if ($dependencyKey !== $selfKey) { + if (! isset($excludedKeys[$dependencyKey])) { $used[$dependencyKey] = true; } } @@ -802,14 +819,23 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e } } + // A dynamic `new` whose target could not be resolved statically (e.g. + // `new $class` on a function parameter) may instantiate any class the + // scanned code references — `$factory->make(X::class)` style call + // sites make X referenced, so treating referenced classes as possibly + // instantiated keeps them concrete without exempting classes nothing + // references at all. + $hasUnresolvedInstantiation = isset($instantiated[ClassCollector::UNRESOLVED_INSTANTIATION]); + foreach ($classNodes as $classNode) { $classNameKey = strtolower($classNode->className); + $isReferenced = isset($used[$classNameKey]) || isset($instantiated[$classNameKey]); - if (isset($used[$classNameKey]) || isset($instantiated[$classNameKey])) { + if ($isReferenced) { $classNode->setReferenced(true); } - if (isset($instantiated[$classNameKey])) { + if (isset($instantiated[$classNameKey]) || ($hasUnresolvedInstantiation && $isReferenced)) { $classNode->setInstantiated(true); } } diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index b60afb78..1c7def96 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -99,6 +99,15 @@ final class ClassCollector extends NodeVisitorAbstract private const CLASS_LIKE_STRING_PATTERN = '/^[A-Za-z_\x80-\xff][A-Za-z0-9_\x80-\xff]*+(?:\\\\[A-Za-z_\x80-\xff][A-Za-z0-9_\x80-\xff]*+)*+$/'; + /** + * Sentinel recorded in the file instantiations when a dynamic `new` has no + * statically known class-name candidates (e.g. `new $class` on a function + * parameter). Can never collide with a real class name. The analyser then + * treats referenced class-likes as possibly instantiated, so a factory + * target passed in as `X::class` is not fixed into an abstract class. + */ + public const UNRESOLVED_INSTANTIATION = '*'; + /** @var list */ private array $nodes = []; @@ -126,6 +135,14 @@ final class ClassCollector extends NodeVisitorAbstract */ private array $variableClassNames = []; + /** + * Variables that received at least one statically unresolvable assignment, + * so `new $variable` cannot claim to know every possible target. + * + * @var array + */ + private array $variableUnknownAssignments = []; + private readonly ConstExprEvaluator $constExprEvaluator; /** @@ -186,18 +203,19 @@ public function __construct( public function setCurrentFile(string $file): void { - $this->currentFile = $file; - $this->currentFileReferences = []; - $this->currentFileInstantiations = []; - $this->variableClassNames = []; - $this->currentNamespaceUses = []; - $this->fileClassLikes = []; - $this->fileFunctions = []; - $this->classLikeAnalysis = []; - $this->classLikeMethods = []; - $this->activeClassLikeAnalyses = []; - $this->activeMethodIds = []; - $this->methodClassLikeAnalyses = []; + $this->currentFile = $file; + $this->currentFileReferences = []; + $this->currentFileInstantiations = []; + $this->variableClassNames = []; + $this->variableUnknownAssignments = []; + $this->currentNamespaceUses = []; + $this->fileClassLikes = []; + $this->fileFunctions = []; + $this->classLikeAnalysis = []; + $this->classLikeMethods = []; + $this->activeClassLikeAnalyses = []; + $this->activeMethodIds = []; + $this->methodClassLikeAnalyses = []; } /** @return list */ @@ -587,10 +605,18 @@ private function collectInstantiation(New_ $new): void // variable may hold — conditional reassignments make every recorded // possibility reachable at runtime. if ($class instanceof Variable && is_string($class->name)) { - foreach (array_keys($this->variableClassNames[$class->name] ?? []) as $possibleClassName) { + $possibleClassNames = array_keys($this->variableClassNames[$class->name] ?? []); + + foreach ($possibleClassNames as $possibleClassName) { $this->currentFileInstantiations[] = $possibleClassName; } + // A variable with no candidates (e.g. a function parameter) or + // with an unresolvable assignment may target any class. + if ($possibleClassNames === [] || isset($this->variableUnknownAssignments[$class->name])) { + $this->currentFileInstantiations[] = self::UNRESOLVED_INSTANTIATION; + } + return; } @@ -599,7 +625,11 @@ private function collectInstantiation(New_ $new): void if ($className !== null) { $this->currentFileInstantiations[] = $className; + + return; } + + $this->currentFileInstantiations[] = self::UNRESOLVED_INSTANTIATION; } /** @@ -648,6 +678,8 @@ private function trackVariableClassName(Assign $assign): void $className = $this->resolveClassNameExpr($assign->expr); if ($className === null) { + $this->variableUnknownAssignments[$assign->var->name] = true; + return; } diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index 0d35e8a0..b1b71b40 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -617,6 +617,57 @@ public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnConditi $this->assertCount(0, $violations); } + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesWhenFactoryInstantiatesUnknownClass(): void + { + // `make(Base::class)` instantiates Base at runtime, but the collector + // cannot connect the argument to `new $class`. The unresolved dynamic + // instantiation makes every referenced class count as possibly + // instantiated. + $functions = 'makeTempProject([ + 'src/Base.php' => ' ' $functions, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + $this->assertCount(0, $violations); + } + + public function testExtendedClassMustBeAbstractOrInstantiatedRuleStillFlagsUnreferencedParent(): void + { + // The unresolved dynamic instantiation exempts only referenced + // classes — an extended class nothing references cannot be its + // target, so it is still reported. + $functions = 'makeTempProject([ + 'src/UnreferencedBase.php' => ' ' $functions, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + $this->assertCount(1, $violations); + $this->assertSame('App\UnreferencedBase', $violations[0]->className); + } + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnSelfAndParentInstantiation(): void { // `new self()` resolves to the class itself even when called through a diff --git a/tests/Analyser/ClassCollectorTest.php b/tests/Analyser/ClassCollectorTest.php index 26f0f5ca..462e1f0c 100644 --- a/tests/Analyser/ClassCollectorTest.php +++ b/tests/Analyser/ClassCollectorTest.php @@ -201,10 +201,38 @@ public function testKeepsEarlierClassNameOnUnresolvableReassignment(): void $classCollector = $this->makeCollector($code); // The reassignment cannot be evaluated statically, but the earlier - // constant value may still reach the instantiation — keeping it is - // the safe over-approximation. + // constant value may still reach the instantiation — keeping it plus + // the unresolved marker is the safe over-approximation. $this->assertSame( - ['/fake/path/Foo.php' => ['App\Base']], + ['/fake/path/Foo.php' => ['App\Base', ClassCollector::UNRESOLVED_INSTANTIATION]], + $classCollector->getFileInstantiations() + ); + } + + public function testMarksUnresolvedInstantiationForUnknownDynamicClass(): void + { + // The class name flows in from a call site the collector cannot see. + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => [ClassCollector::UNRESOLVED_INSTANTIATION]], + $classCollector->getFileInstantiations() + ); + } + + public function testMarksUnresolvedInstantiationForRuntimeClassExpression(): void + { + $code = 'class)(); } }'; + + $classCollector = $this->makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => [ClassCollector::UNRESOLVED_INSTANTIATION]], $classCollector->getFileInstantiations() ); } @@ -219,20 +247,23 @@ public function testIgnoresUnresolvableClassNameExpressions(): void $classCollector = $this->makeCollector($code); // Runtime-dependent concatenation, non-class-shaped strings, and - // property assignments resolve to nothing. - $this->assertSame([], $classCollector->getFileInstantiations()); + // property assignments resolve to no concrete candidate — only the + // unresolved marker remains. + $this->assertSame( + ['/fake/path/Foo.php' => [ClassCollector::UNRESOLVED_INSTANTIATION]], + $classCollector->getFileInstantiations() + ); } - public function testDoesNotCollectDynamicOrAnonymousInstantiations(): void + public function testDoesNotCollectAnonymousInstantiations(): void { $code = 'makeCollector($code); - // `new $class` has no resolvable name (class-name strings cover it), - // and anonymous classes are tracked as AnonymousClassNodes. + // Anonymous classes are tracked as AnonymousClassNodes, and their + // known declaration does not make any named class instantiable. $this->assertSame([], $classCollector->getFileInstantiations()); } From 44ab1b3edd8719d860bf4c340141cf4a3307db2a Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 20 Aug 2026 00:05:44 +0700 Subject: [PATCH 15/19] more checks --- docs/available-rules.md | 2 +- src/Analyser/ClassCollector.php | 29 +++++++++++++++++++++++++++ tests/Analyser/AnalyserTest.php | 25 +++++++++++++++++++++++ tests/Analyser/ClassCollectorTest.php | 27 +++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/docs/available-rules.md b/docs/available-rules.md index 215b8ac8..1b26e46e 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -78,7 +78,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `ClassNameMustBeStudlyCapsRule` | `new ClassNameMustBeStudlyCapsRule(layer: 'Source')` | Class names use StudlyCaps. | | `ClassNameMustHaveSuffixRule` | `new ClassNameMustHaveSuffixRule(layer: 'Controller', suffix: 'Controller')` | Classes in a layer have the required suffix. | | `ClassNameMustNotHavePrefixRule` | `new ClassNameMustNotHavePrefixRule(layer: 'Model', prefix: 'Model')` | Classes in a layer do not use a forbidden prefix. | -| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count — but when the scanned code contains a dynamic `new` whose target cannot be resolved (e.g. a factory's `new $class` fed by call sites), referenced classes conservatively count as instantiated. Supports `--fix` by adding the `abstract` modifier. | +| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count — but when the scanned code contains a dynamic instantiation whose target cannot be resolved (a factory's `new $class` fed by call sites, or a `ReflectionClass::newInstance*()` call), referenced classes conservatively count as instantiated. Supports `--fix` by adding the `abstract` modifier. | | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index 1c7def96..d2042a60 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -24,7 +24,9 @@ use PhpParser\Node\Expr\Include_; use PhpParser\Node\Expr\Isset_; use PhpParser\Node\Expr\List_; +use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\New_; +use PhpParser\Node\Expr\NullsafeMethodCall; use PhpParser\Node\Expr\Print_; use PhpParser\Node\Expr\Ternary; use PhpParser\Node\Expr\Variable; @@ -108,6 +110,21 @@ final class ClassCollector extends NodeVisitorAbstract */ public const UNRESOLVED_INSTANTIATION = '*'; + /** + * Method names of the ReflectionClass object-construction APIs. Calling + * any of them instantiates a class the collector cannot determine, so the + * call records {@see self::UNRESOLVED_INSTANTIATION}. Matching by method + * name alone over-approximates (any receiver type matches) — the safe + * direction for a fixer that would otherwise make a class abstract. + */ + private const REFLECTION_CONSTRUCTION_METHODS = [ + 'newinstance' => true, + 'newinstanceargs' => true, + 'newinstancewithoutconstructor' => true, + 'newlazyghost' => true, + 'newlazyproxy' => true, + ]; + /** @var list */ private array $nodes = []; @@ -463,6 +480,18 @@ private function collectNodeAnalysis(Node $node): void return; } + // Reflection construction APIs instantiate a class the collector + // cannot pin down statically, exactly like an unresolvable + // `new $class` — the unresolved marker keeps referenced classes + // concrete. + if ( + ($node instanceof MethodCall || $node instanceof NullsafeMethodCall) + && $node->name instanceof Identifier + && isset(self::REFLECTION_CONSTRUCTION_METHODS[$node->name->toLowerString()]) + ) { + $this->currentFileInstantiations[] = self::UNRESOLVED_INSTANTIATION; + } + if ($this->activeClassLikeAnalyses === []) { // Outside any named class-like scope — procedural functions, // top-level statements, top-level anonymous class bodies — a diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index b1b71b40..3c990831 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -643,6 +643,31 @@ public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesWhenFacto $this->assertCount(0, $violations); } + public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnReflectionInstantiation(): void + { + // ReflectionClass::newInstance() constructs Base at runtime; the + // reflection construction call marks an unresolved instantiation, so + // the referenced Base stays concrete. + $bootstrap = 'newInstance();'; + + $basePath = $this->makeTempProject([ + 'src/Base.php' => ' ' $bootstrap, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + $this->assertCount(0, $violations); + } + public function testExtendedClassMustBeAbstractOrInstantiatedRuleStillFlagsUnreferencedParent(): void { // The unresolved dynamic instantiation exempts only referenced diff --git a/tests/Analyser/ClassCollectorTest.php b/tests/Analyser/ClassCollectorTest.php index 462e1f0c..b64e0ca4 100644 --- a/tests/Analyser/ClassCollectorTest.php +++ b/tests/Analyser/ClassCollectorTest.php @@ -223,6 +223,33 @@ public function testMarksUnresolvedInstantiationForUnknownDynamicClass(): void ); } + public function testMarksUnresolvedInstantiationForReflectionConstruction(): void + { + // ReflectionClass::newInstance() constructs an object of a class the + // collector cannot determine. + $code = 'newInstance() ?? $n?->newInstanceWithoutConstructor(); } }'; + + $classCollector = $this->makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => [ClassCollector::UNRESOLVED_INSTANTIATION]], + $classCollector->getFileInstantiations() + ); + } + + public function testDoesNotMarkUnresolvedInstantiationForOrdinaryMethodCalls(): void + { + $code = 'handle(); } }'; + + $classCollector = $this->makeCollector($code); + + $this->assertSame([], $classCollector->getFileInstantiations()); + } + public function testMarksUnresolvedInstantiationForRuntimeClassExpression(): void { $code = ' Date: Thu, 20 Aug 2026 00:10:56 +0700 Subject: [PATCH 16/19] add more checks --- docs/available-rules.md | 2 +- src/Analyser/Analyser.php | 19 ++++++++------- src/Analyser/ClassCollector.php | 14 ++++++++--- tests/Analyser/AnalyserTest.php | 35 ++++++++++++++++++++++----- tests/Analyser/ClassCollectorTest.php | 14 +++++++++++ 5 files changed, 65 insertions(+), 19 deletions(-) diff --git a/docs/available-rules.md b/docs/available-rules.md index 1b26e46e..8b1fa18c 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -78,7 +78,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `ClassNameMustBeStudlyCapsRule` | `new ClassNameMustBeStudlyCapsRule(layer: 'Source')` | Class names use StudlyCaps. | | `ClassNameMustHaveSuffixRule` | `new ClassNameMustHaveSuffixRule(layer: 'Controller', suffix: 'Controller')` | Classes in a layer have the required suffix. | | `ClassNameMustNotHavePrefixRule` | `new ClassNameMustNotHavePrefixRule(layer: 'Model', prefix: 'Model')` | Classes in a layer do not use a forbidden prefix. | -| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count — but when the scanned code contains a dynamic instantiation whose target cannot be resolved (a factory's `new $class` fed by call sites, or a `ReflectionClass::newInstance*()` call), referenced classes conservatively count as instantiated. Supports `--fix` by adding the `abstract` modifier. | +| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count — but when the scanned code contains a dynamic instantiation whose target cannot be resolved (a factory's `new $class` fed by call sites, a `ReflectionClass::newInstance*()` call, or `unserialize()`), every class conservatively counts as instantiated and the rule stays silent — the target name may come from environment or configuration, so no class can be proven safe to abstract. Supports `--fix` by adding the `abstract` modifier. | | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 66403599..0780e237 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -819,23 +819,24 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e } } - // A dynamic `new` whose target could not be resolved statically (e.g. - // `new $class` on a function parameter) may instantiate any class the - // scanned code references — `$factory->make(X::class)` style call - // sites make X referenced, so treating referenced classes as possibly - // instantiated keeps them concrete without exempting classes nothing - // references at all. + // A dynamic instantiation whose target could not be resolved + // statically — `new $class` on a function parameter, a + // ReflectionClass construction, unserialize() — may target any class: + // the name can come from the environment, configuration, or a + // payload, entirely outside the scanned code. No class can then be + // proven safe to abstract, so every class-like conservatively counts + // as instantiated. Resolvable construction keeps precise detection; + // unresolvable construction silences the concreteness fix. $hasUnresolvedInstantiation = isset($instantiated[ClassCollector::UNRESOLVED_INSTANTIATION]); foreach ($classNodes as $classNode) { $classNameKey = strtolower($classNode->className); - $isReferenced = isset($used[$classNameKey]) || isset($instantiated[$classNameKey]); - if ($isReferenced) { + if (isset($used[$classNameKey]) || isset($instantiated[$classNameKey])) { $classNode->setReferenced(true); } - if (isset($instantiated[$classNameKey]) || ($hasUnresolvedInstantiation && $isReferenced)) { + if ($hasUnresolvedInstantiation || isset($instantiated[$classNameKey])) { $classNode->setInstantiated(true); } } diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index d2042a60..0991994c 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -480,9 +480,9 @@ private function collectNodeAnalysis(Node $node): void return; } - // Reflection construction APIs instantiate a class the collector - // cannot pin down statically, exactly like an unresolvable - // `new $class` — the unresolved marker keeps referenced classes + // Reflection construction APIs and unserialize() instantiate a class + // the collector cannot pin down statically, exactly like an + // unresolvable `new $class` — the unresolved marker keeps classes // concrete. if ( ($node instanceof MethodCall || $node instanceof NullsafeMethodCall) @@ -492,6 +492,14 @@ private function collectNodeAnalysis(Node $node): void $this->currentFileInstantiations[] = self::UNRESOLVED_INSTANTIATION; } + if ( + $node instanceof FuncCall + && $node->name instanceof Name + && $node->name->toLowerString() === 'unserialize' + ) { + $this->currentFileInstantiations[] = self::UNRESOLVED_INSTANTIATION; + } + if ($this->activeClassLikeAnalyses === []) { // Outside any named class-like scope — procedural functions, // top-level statements, top-level anonymous class bodies — a diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index 3c990831..c78f1525 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -668,11 +668,12 @@ public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnReflect $this->assertCount(0, $violations); } - public function testExtendedClassMustBeAbstractOrInstantiatedRuleStillFlagsUnreferencedParent(): void + public function testExtendedClassMustBeAbstractOrInstantiatedRuleSkipsAllWhenUnresolvedInstantiationExists(): void { - // The unresolved dynamic instantiation exempts only referenced - // classes — an extended class nothing references cannot be its - // target, so it is still reported. + // `make($_ENV['CLASS'])` can name any class — even one nothing in the + // scanned code references — so an unresolved dynamic instantiation + // must silence the rule entirely: no class can be proven safe to + // abstract. $functions = 'analyse($architecture, [], null, AnalyserOptions::sequential()) ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); - $this->assertCount(1, $violations); - $this->assertSame('App\UnreferencedBase', $violations[0]->className); + $this->assertCount(0, $violations); + } + + public function testExtendedClassMustBeAbstractOrInstantiatedRuleSkipsAllWhenUnserializeIsCalled(): void + { + // unserialize() constructs instances of whatever the payload names; + // abstracting a serialized concrete class breaks deserialization. + $bootstrap = 'makeTempProject([ + 'src/Base.php' => ' ' $bootstrap, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + $this->assertCount(0, $violations); } public function testExtendedClassMustBeAbstractOrInstantiatedRulePassesOnSelfAndParentInstantiation(): void diff --git a/tests/Analyser/ClassCollectorTest.php b/tests/Analyser/ClassCollectorTest.php index b64e0ca4..662eccfb 100644 --- a/tests/Analyser/ClassCollectorTest.php +++ b/tests/Analyser/ClassCollectorTest.php @@ -239,6 +239,20 @@ public function testMarksUnresolvedInstantiationForReflectionConstruction(): voi ); } + public function testMarksUnresolvedInstantiationForUnserialize(): void + { + $code = 'makeCollector($code); + + $this->assertSame( + ['/fake/path/Foo.php' => [ClassCollector::UNRESOLVED_INSTANTIATION]], + $classCollector->getFileInstantiations() + ); + } + public function testDoesNotMarkUnresolvedInstantiationForOrdinaryMethodCalls(): void { $code = ' Date: Thu, 20 Aug 2026 00:15:55 +0700 Subject: [PATCH 17/19] add check on eval-a --- docs/available-rules.md | 2 +- src/Analyser/ClassCollector.php | 6 ++++++ tests/Analyser/AnalyserTest.php | 22 ++++++++++++++++++++++ tests/Analyser/ClassCollectorTest.php | 16 ++++++++++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) diff --git a/docs/available-rules.md b/docs/available-rules.md index 8b1fa18c..194fca92 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -78,7 +78,7 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | `ClassNameMustBeStudlyCapsRule` | `new ClassNameMustBeStudlyCapsRule(layer: 'Source')` | Class names use StudlyCaps. | | `ClassNameMustHaveSuffixRule` | `new ClassNameMustHaveSuffixRule(layer: 'Controller', suffix: 'Controller')` | Classes in a layer have the required suffix. | | `ClassNameMustNotHavePrefixRule` | `new ClassNameMustNotHavePrefixRule(layer: 'Model', prefix: 'Model')` | Classes in a layer do not use a forbidden prefix. | -| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count — but when the scanned code contains a dynamic instantiation whose target cannot be resolved (a factory's `new $class` fed by call sites, a `ReflectionClass::newInstance*()` call, or `unserialize()`), every class conservatively counts as instantiated and the rule stays silent — the target name may come from environment or configuration, so no class can be proven safe to abstract. Supports `--fix` by adding the `abstract` modifier. | +| `ExtendedClassMustBeAbstractOrInstantiatedRule` | `new ExtendedClassMustBeAbstractOrInstantiatedRule(layer: 'Source')` | Classes another scanned class extends are declared `abstract` unless they are also instantiated (`new X`, a `new self`/`new static`/`new parent` resolving to them, or a dynamic `new $class` whose class name resolves from a constant expression such as `X::class` or a class-name string). Type hints, `instanceof`, and `::class` keep working on an abstract class, so they do not count — but when the scanned code contains a dynamic instantiation whose target cannot be resolved (a factory's `new $class` fed by call sites, a `ReflectionClass::newInstance*()` call, `unserialize()`, or `eval()`), every class conservatively counts as instantiated and the rule stays silent — the target name may come from environment or configuration, so no class can be proven safe to abstract. Supports `--fix` by adding the `abstract` modifier. | | `MaxDependencyCountRule` | `new MaxDependencyCountRule(layer: 'Controller', maxCount: 5)` | Constructor dependency count stays below the configured limit. | | `MayNotImplementInterfaceRule` | `new MayNotImplementInterfaceRule(layer: 'Domain', interface: JsonSerializable::class)` | Classes in a layer do not implement a forbidden interface. | | `MustBeFinalRule` | `new MustBeFinalRule(layer: 'Domain', classNamePattern: '/Entity$/')` | Matching classes in a layer are declared `final`. Classes extended by another scanned class are skipped (making them `final` would break the child). Supports `--fix`. | diff --git a/src/Analyser/ClassCollector.php b/src/Analyser/ClassCollector.php index 0991994c..be9ed4c3 100644 --- a/src/Analyser/ClassCollector.php +++ b/src/Analyser/ClassCollector.php @@ -500,6 +500,12 @@ private function collectNodeAnalysis(Node $node): void $this->currentFileInstantiations[] = self::UNRESOLVED_INSTANTIATION; } + // eval() can construct anything; it is additionally recorded as a + // language construct for in-class usage rules further down. + if ($node instanceof Eval_) { + $this->currentFileInstantiations[] = self::UNRESOLVED_INSTANTIATION; + } + if ($this->activeClassLikeAnalyses === []) { // Outside any named class-like scope — procedural functions, // top-level statements, top-level anonymous class bodies — a diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index c78f1525..2e419f6f 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -693,6 +693,28 @@ public function testExtendedClassMustBeAbstractOrInstantiatedRuleSkipsAllWhenUnr $this->assertCount(0, $violations); } + public function testExtendedClassMustBeAbstractOrInstantiatedRuleSkipsAllWhenEvalIsUsed(): void + { + // eval() can construct any class the evaluated code names. + $bootstrap = 'makeTempProject([ + 'src/Base.php' => ' ' $bootstrap, + ]); + + $architecture = Architecture::define() + ->withPreset(Preset::YAGNI(sourcePaths: ['src/'])); + + $violations = (new Analyser($basePath)) + ->analyse($architecture, [], null, AnalyserOptions::sequential()) + ->forRule(YagniPreset::EXTENDED_CLASS_MUST_BE_ABSTRACT_OR_INSTANTIATED); + + $this->assertCount(0, $violations); + } + public function testExtendedClassMustBeAbstractOrInstantiatedRuleSkipsAllWhenUnserializeIsCalled(): void { // unserialize() constructs instances of whatever the payload names; diff --git a/tests/Analyser/ClassCollectorTest.php b/tests/Analyser/ClassCollectorTest.php index 662eccfb..45c7b278 100644 --- a/tests/Analyser/ClassCollectorTest.php +++ b/tests/Analyser/ClassCollectorTest.php @@ -253,6 +253,22 @@ public function testMarksUnresolvedInstantiationForUnserialize(): void ); } + public function testMarksUnresolvedInstantiationForEval(): void + { + $inClass = 'assertSame( + ['/fake/path/Foo.php' => [ClassCollector::UNRESOLVED_INSTANTIATION]], + $this->makeCollector($inClass)->getFileInstantiations() + ); + $this->assertSame( + ['/fake/path/Foo.php' => [ClassCollector::UNRESOLVED_INSTANTIATION]], + $this->makeCollector($procedural)->getFileInstantiations() + ); + } + public function testDoesNotMarkUnresolvedInstantiationForOrdinaryMethodCalls(): void { $code = ' Date: Thu, 20 Aug 2026 00:34:03 +0700 Subject: [PATCH 18/19] avoid mark instantiated on abstract class, trait, interface, enum --- src/Analyser/ClassNode.php | 9 ++++++++ tests/Analyser/ClassNodeTest.php | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/Analyser/ClassNode.php b/src/Analyser/ClassNode.php index 4e8e4ce3..68d7e081 100644 --- a/src/Analyser/ClassNode.php +++ b/src/Analyser/ClassNode.php @@ -113,9 +113,18 @@ public function setReferenced(bool $isReferenced): void * `new self`/`new static`/`new parent` resolving to it. Instantiation is * the one usage that requires a class to stay concrete. Computed by the * analyser when a usage-aware rule is active; false otherwise. + * + * Only a concrete named class can be an instantiation target — `new` on + * an abstract class, interface, trait, or enum is fatal — so marking any + * other class-like as instantiated is ignored. (Anonymous classes never + * become ClassNodes in the first place.) */ public function setInstantiated(bool $isInstantiated): void { + if ($isInstantiated && (! $this->isClass() || $this->isAbstract)) { + return; + } + $this->isInstantiated = $isInstantiated; } diff --git a/tests/Analyser/ClassNodeTest.php b/tests/Analyser/ClassNodeTest.php index 992b7f33..8230a57b 100644 --- a/tests/Analyser/ClassNodeTest.php +++ b/tests/Analyser/ClassNodeTest.php @@ -339,6 +339,43 @@ className: 'App\\Domain\\BaseRepository', $this->assertFalse($classNode->isInstantiated); } + public function testSetInstantiatedIsIgnoredForNonInstantiableClassLikes(): void + { + $makeNode = static fn ( + bool $isAbstract = false, + bool $isInterface = false, + bool $isTrait = false, + bool $isEnum = false, + ): ClassNode => new ClassNode( + className: 'App\\Domain\\SomeClassLike', + file: '/src/SomeClassLike.php', + line: 5, + layer: 'Domain', + extends: null, + isAbstract: $isAbstract, + isFinal: false, + isInterface: $isInterface, + isReadonly: false, + isTrait: $isTrait, + isEnum: $isEnum, + ); + + $nonInstantiables = [ + 'abstract class' => $makeNode(isAbstract: true), + 'interface' => $makeNode(isInterface: true), + 'trait' => $makeNode(isTrait: true), + 'enum' => $makeNode(isEnum: true), + ]; + + foreach ($nonInstantiables as $kind => $classNode) { + $classNode->setInstantiated(true); + + // `new` on these class-likes is fatal, so they can never be an + // instantiation target. + $this->assertFalse($classNode->isInstantiated, $kind); + } + } + public function testDependsOnMatchesExistingClassesExactly(): void { $classNode = new ClassNode( From d31677af2170699197ca0734552d6c1e5c3d38b7 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 20 Aug 2026 00:38:22 +0700 Subject: [PATCH 19/19] reduce complexity --- src/Analyser/Analyser.php | 61 +++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 0780e237..8da418c4 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -172,6 +172,10 @@ public function analyse( $this->markReferencedClassLikes($classNodes, $extractionResult); } + if ($hasExtendedClassAwareRule) { + $this->markInstantiatedClasses($classNodes, $extractionResult); + } + if ($withFileAnalysis) { $fileAnalysisProvider = new FileAnalysisProvider( analyses: $extractionResult->fileAnalyses, @@ -807,10 +811,43 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e } } - // Instantiations (`new X`, with self/static/parent already resolved) - // are the one usage that requires a class to stay concrete, so they - // are tracked apart from plain references. No self-exclusion here: a - // class instantiating itself cannot become abstract either. + // An instantiation is also a reference; the sentinel is not a class + // name and marks nothing here. + foreach ($extractionResult->fileInstantiations as $instantiations) { + foreach ($instantiations as $instantiation) { + if ($instantiation !== ClassCollector::UNRESOLVED_INSTANTIATION) { + $used[strtolower($instantiation)] = true; + } + } + } + + foreach ($classNodes as $classNode) { + if (isset($used[strtolower($classNode->className)])) { + $classNode->setReferenced(true); + } + } + } + + /** + * Flag every concrete class that another scanned scope instantiates — + * `new X` (with self/static/parent already resolved), a dynamic + * `new $class` resolved from constant class-name values, and so on. + * No self-exclusion here: a class instantiating itself cannot become + * abstract either. + * + * A dynamic instantiation whose target could not be resolved statically — + * `new $class` on a function parameter, a ReflectionClass construction, + * unserialize(), eval() — may target any class: the name can come from + * the environment, configuration, or a payload, entirely outside the + * scanned code. No class can then be proven safe to abstract, so every + * class-like conservatively counts as instantiated. Resolvable + * construction keeps precise detection; unresolvable construction + * silences the concreteness fix. + * + * @param list $classNodes + */ + private function markInstantiatedClasses(array $classNodes, ExtractionResult $extractionResult): void + { $instantiated = []; foreach ($extractionResult->fileInstantiations as $instantiations) { @@ -819,24 +856,10 @@ private function markReferencedClassLikes(array $classNodes, ExtractionResult $e } } - // A dynamic instantiation whose target could not be resolved - // statically — `new $class` on a function parameter, a - // ReflectionClass construction, unserialize() — may target any class: - // the name can come from the environment, configuration, or a - // payload, entirely outside the scanned code. No class can then be - // proven safe to abstract, so every class-like conservatively counts - // as instantiated. Resolvable construction keeps precise detection; - // unresolvable construction silences the concreteness fix. $hasUnresolvedInstantiation = isset($instantiated[ClassCollector::UNRESOLVED_INSTANTIATION]); foreach ($classNodes as $classNode) { - $classNameKey = strtolower($classNode->className); - - if (isset($used[$classNameKey]) || isset($instantiated[$classNameKey])) { - $classNode->setReferenced(true); - } - - if ($hasUnresolvedInstantiation || isset($instantiated[$classNameKey])) { + if ($hasUnresolvedInstantiation || isset($instantiated[strtolower($classNode->className)])) { $classNode->setInstantiated(true); } }