[TASK] Add (and use) a CSSListItem type (#1212)

This allows a single type to be used for the contents of a `CSSList`,
instead of a long list of orred types, and helps with static analysis.

Various `assertInstanceOf()` tests are added to the test cases to confirm that
the list items are of the type expected.

Some `implements` and `exetends` lists are now alphabetically sorted.

Also don't implement interfaces extended by another that is also implemented

PHP<7.4 does not allow this.
Instead, for clarity,
add a DocBlock comment stating which additional interfaces should be
implemented that are not explicitly listed in the `implements` section.

When our minimum PHP version becomes 7.4 or above, we can revisit this.

Author: JakeQZ

config/phpstan-baseline.neon

@@ -18,6 +18,12 @@ parameters:
 			count: 1
 			path: ../src/CSSList/CSSList.php
 
+		-
+			message: '#^Parameters should have "Sabberworm\\CSS\\CSSList\\CSSListItem\|array" types as the only types passed to this method$#'
+			identifier: typePerfect.narrowPublicClassMethodParamType
+			count: 1
+			path: ../src/CSSList/CSSList.php
+
 		-
 			message: '#^Short ternary operator is not allowed\. Use null coalesce operator if applicable or consider using long ternary\.$#'
 			identifier: ternary.shortNotAllowed
@@ -54,12 +60,6 @@ parameters:
 			count: 2
 			path: ../src/RuleSet/RuleSet.php
 
-		-
-			message: '#^Parameters should have "Sabberworm\\CSS\\Rule\\Rule" types as the only types passed to this method$#'
-			identifier: typePerfect.narrowPublicClassMethodParamType
-			count: 1
-			path: ../src/RuleSet/RuleSet.php
-
 		-
 			message: '#^Parameters should have "string" types as the only types passed to this method$#'
 			identifier: typePerfect.narrowPublicClassMethodParamType

src/CSSList/CSSList.php

@@ -30,8 +30,11 @@
  * `RuleSet`s as well as other `CSSList` objects.
  *
  * It can also contain `Import` and `Charset` objects stemming from at-rules.
+ *
+ * Note that `CSSListItem` extends both `Commentable` and `Renderable`,
+ * so those interfaces must also be implemented by concrete subclasses.
  */
-abstract class CSSList implements Renderable, Commentable
+abstract class CSSList implements CSSListItem
 {
     /**
      * @var list<Comment>
@@ -41,7 +44,7 @@ abstract class CSSList implements Renderable, Commentable
     protected $comments = [];
 
     /**
-     * @var array<int<0, max>, RuleSet|CSSList|Import|Charset>
+     * @var array<int<0, max>, CSSListItem>
      *
      * @internal since 8.8.0
      */
@@ -105,7 +108,7 @@ public static function parseList(ParserState $parserState, CSSList $list): void
     }
 
     /**
-     * @return AtRuleBlockList|KeyFrame|Charset|CSSNamespace|Import|AtRuleSet|DeclarationBlock|false|null
+     * @return CSSListItem|false|null
      *         If `null` is returned, it means the end of the list has been reached.
      *         If `false` is returned, it means an invalid item has been encountered,
      *         but parsing of the next item should proceed.
@@ -156,13 +159,11 @@ private static function parseListItem(ParserState $parserState, CSSList $list)
     }
 
     /**
-     * @return AtRuleBlockList|KeyFrame|Charset|CSSNamespace|Import|AtRuleSet|null
-     *
      * @throws SourceException
      * @throws UnexpectedTokenException
      * @throws UnexpectedEOFException
      */
-    private static function parseAtRule(ParserState $parserState)
+    private static function parseAtRule(ParserState $parserState): ?CSSListItem
     {
         $parserState->consume('@');
         $identifier = $parserState->parseIdentifier();
@@ -265,28 +266,24 @@ public function getLineNo(): int
 
     /**
      * Prepends an item to the list of contents.
-     *
-     * @param RuleSet|CSSList|Import|Charset $item
      */
-    public function prepend($item): void
+    public function prepend(CSSListItem $item): void
     {
         \array_unshift($this->contents, $item);
     }
 
     /**
      * Appends an item to the list of contents.
-     *
-     * @param RuleSet|CSSList|Import|Charset $item
      */
-    public function append($item): void
+    public function append(CSSListItem $item): void
     {
         $this->contents[] = $item;
     }
 
     /**
      * Splices the list of contents.
      *
-     * @param array<int, RuleSet|CSSList|Import|Charset> $replacement
+     * @param array<int, CSSListItem> $replacement
      */
     public function splice(int $offset, ?int $length = null, ?array $replacement = null): void
     {
@@ -296,11 +293,8 @@ public function splice(int $offset, ?int $length = null, ?array $replacement = n
     /**
      * Inserts an item in the CSS list before its sibling. If the desired sibling cannot be found,
      * the item is appended at the end.
-     *
-     * @param RuleSet|CSSList|Import|Charset $item
-     * @param RuleSet|CSSList|Import|Charset $sibling
      */
-    public function insertBefore($item, $sibling): void
+    public function insertBefore(CSSListItem $item, CSSListItem $sibling): void
     {
         if (\in_array($sibling, $this->contents, true)) {
             $this->replace($sibling, [$item, $sibling]);
@@ -312,13 +306,13 @@ public function insertBefore($item, $sibling): void
     /**
      * Removes an item from the CSS list.
      *
-     * @param RuleSet|Import|Charset|CSSList $itemToRemove
+     * @param CSSListItem $itemToRemove
      *        May be a `RuleSet` (most likely a `DeclarationBlock`), an `Import`,
      *        a `Charset` or another `CSSList` (most likely a `MediaQuery`)
      *
      * @return bool whether the item was removed
      */
-    public function remove($itemToRemove): bool
+    public function remove(CSSListItem $itemToRemove): bool
     {
         $key = \array_search($itemToRemove, $this->contents, true);
         if ($key !== false) {
@@ -332,12 +326,12 @@ public function remove($itemToRemove): bool
     /**
      * Replaces an item from the CSS list.
      *
-     * @param RuleSet|Import|Charset|CSSList $oldItem
+     * @param CSSListItem $oldItem
      *        May be a `RuleSet` (most likely a `DeclarationBlock`), an `Import`, a `Charset`
      *        or another `CSSList` (most likely a `MediaQuery`)
-     * @param RuleSet|Import|Charset|CSSList|array<RuleSet|Import|Charset|CSSList> $newItem
+     * @param CSSListItem|array<CSSListItem> $newItem
      */
-    public function replace($oldItem, $newItem): bool
+    public function replace(CSSListItem $oldItem, $newItem): bool
     {
         $key = \array_search($oldItem, $this->contents, true);
         if ($key !== false) {
@@ -353,7 +347,7 @@ public function replace($oldItem, $newItem): bool
     }
 
     /**
-     * @param array<int, RuleSet|Import|Charset|CSSList> $contents
+     * @param array<int, CSSListItem> $contents
      */
     public function setContents(array $contents): void
     {
@@ -444,7 +438,7 @@ abstract public function isRootList(): bool;
     /**
      * Returns the stored items.
      *
-     * @return array<int<0, max>, RuleSet|Import|Charset|CSSList>
+     * @return array<int<0, max>, CSSListItem>
      */
     public function getContents(): array
     {

src/CSSList/CSSListItem.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sabberworm\CSS\CSSList;
+
+use Sabberworm\CSS\Comment\Commentable;
+use Sabberworm\CSS\Renderable;
+
+/**
+ * Represents anything that can be in the `$contents` of a `CSSList`.
+ *
+ * The interface does not define any methods to implement.
+ * It's purpose is to allow a single type to be specified for `CSSList::$contents` and manipulation methods thereof.
+ * It extends `Commentable` and `Renderable` because all `CSSListItem`s are both.
+ * This allows implementations to call methods from those interfaces without any additional type checks.
+ */
+interface CSSListItem extends Commentable, Renderable {}

src/Property/AtRule.php

@@ -5,9 +5,14 @@
 namespace Sabberworm\CSS\Property;
 
 use Sabberworm\CSS\Comment\Commentable;
+use Sabberworm\CSS\CSSList\CSSListItem;
 use Sabberworm\CSS\Renderable;
 
-interface AtRule extends Renderable, Commentable
+/**
+ * Note that `CSSListItem` extends both `Commentable` and `Renderable`,
+ * so concrete classes implementing this interface must also implement those.
+ */
+interface AtRule extends CSSListItem
 {
     /**
      * Since there are more set rules than block rules,

src/RuleSet/RuleSet.php

@@ -6,6 +6,7 @@
 
 use Sabberworm\CSS\Comment\Comment;
 use Sabberworm\CSS\Comment\Commentable;
+use Sabberworm\CSS\CSSList\CSSListItem;
 use Sabberworm\CSS\OutputFormat;
 use Sabberworm\CSS\Parsing\ParserState;
 use Sabberworm\CSS\Parsing\UnexpectedEOFException;
@@ -21,8 +22,11 @@
  *
  * If you want to manipulate a `RuleSet`, use the methods `addRule(Rule $rule)`, `getRules()` and `removeRule($rule)`
  * (which accepts either a `Rule` or a rule name; optionally suffixed by a dash to remove all related rules).
+ *
+ * Note that `CSSListItem` extends both `Commentable` and `Renderable`,
+ * so those interfaces must also be implemented by concrete subclasses.
  */
-abstract class RuleSet implements Renderable, Commentable
+abstract class RuleSet implements CSSListItem
 {
     /**
      * the rules in this rule set, using the property name as the key,

tests/CSSList/AtRuleBlockListTest.php

@@ -5,6 +5,7 @@
 namespace Sabberworm\CSS\Tests\CSSList;
 
 use PHPUnit\Framework\TestCase;
+use Sabberworm\CSS\CSSList\AtRuleBlockList;
 use Sabberworm\CSS\Parser;
 use Sabberworm\CSS\Settings;
 
@@ -60,6 +61,7 @@ public function parsesRuleNameOfMediaQueries(string $css): void
         $contents = (new Parser($css))->parse()->getContents();
         $atRuleBlockList = $contents[0];
 
+        self::assertInstanceOf(AtRuleBlockList::class, $atRuleBlockList);
         self::assertSame('media', $atRuleBlockList->atRuleName());
     }
 
@@ -73,6 +75,7 @@ public function parsesArgumentsOfMediaQueries(string $css): void
         $contents = (new Parser($css))->parse()->getContents();
         $atRuleBlockList = $contents[0];
 
+        self::assertInstanceOf(AtRuleBlockList::class, $atRuleBlockList);
         self::assertSame('(min-width: 768px)', $atRuleBlockList->atRuleArgs());
     }
 

tests/ParserTest.php

@@ -5,6 +5,8 @@
 namespace Sabberworm\CSS\Tests;
 
 use PHPUnit\Framework\TestCase;
+use Sabberworm\CSS\Comment\Commentable;
+use Sabberworm\CSS\CSSList\CSSList;
 use Sabberworm\CSS\CSSList\Document;
 use Sabberworm\CSS\CSSList\KeyFrame;
 use Sabberworm\CSS\OutputFormat;
@@ -973,9 +975,30 @@ public function lineNumbersParsing(): void
 
         $actual = [];
         foreach ($document->getContents() as $contentItem) {
+            // PHPStan can see what `assertInstanceOf()` does,
+            // but does not understand `LogicalOr` with multiple `IsIntanceOf` constraints.
+            // So a more explicit type check is required.
+            // TODO: tidy this up when an interface with `getLineNo()` is added.
+            if (
+                !$contentItem instanceof Charset &&
+                !$contentItem instanceof CSSList &&
+                !$contentItem instanceof CSSNamespace &&
+                !$contentItem instanceof Import &&
+                !$contentItem instanceof RuleSet
+            ) {
+                self::fail('Content item is not of an expected type.  It\'s a `' . \get_class($contentItem) . '`.');
+            }
             $actual[$contentItem->getLineNo()] = [\get_class($contentItem)];
             if ($contentItem instanceof KeyFrame) {
                 foreach ($contentItem->getContents() as $block) {
+                    if (
+                        !$block instanceof CSSList &&
+                        !$block instanceof RuleSet
+                    ) {
+                        self::fail(
+                            'KeyFrame content item is not of an expected type.  It\'s a `' . \get_class($block) . '`.'
+                        );
+                    }
                     $actual[$contentItem->getLineNo()][] = $block->getLineNo();
                 }
             }
@@ -1037,37 +1060,44 @@ public function commentExtracting(): void
         $nodes = $document->getContents();
 
         // Import property.
+        self::assertInstanceOf(Commentable::class, $nodes[0]);
         $importComments = $nodes[0]->getComments();
         self::assertCount(2, $importComments);
         self::assertSame("*\n * Comments\n ", $importComments[0]->getComment());
         self::assertSame(' Hell ', $importComments[1]->getComment());
 
         // Declaration block.
         $fooBarBlock = $nodes[1];
+        self::assertInstanceOf(Commentable::class, $fooBarBlock);
         $fooBarBlockComments = $fooBarBlock->getComments();
         // TODO Support comments in selectors.
         // $this->assertCount(2, $fooBarBlockComments);
         // $this->assertSame("* Number 4 *", $fooBarBlockComments[0]->getComment());
         // $this->assertSame("* Number 5 *", $fooBarBlockComments[1]->getComment());
 
         // Declaration rules.
+        self::assertInstanceOf(RuleSet::class, $fooBarBlock);
         $fooBarRules = $fooBarBlock->getRules();
         $fooBarRule = $fooBarRules[0];
         $fooBarRuleComments = $fooBarRule->getComments();
         self::assertCount(1, $fooBarRuleComments);
         self::assertSame(' Number 6 ', $fooBarRuleComments[0]->getComment());
 
         // Media property.
+        self::assertInstanceOf(Commentable::class, $nodes[2]);
         $mediaComments = $nodes[2]->getComments();
         self::assertCount(0, $mediaComments);
 
         // Media children.
+        self::assertInstanceOf(CSSList::class, $nodes[2]);
         $mediaRules = $nodes[2]->getContents();
+        self::assertInstanceOf(Commentable::class, $mediaRules[0]);
         $fooBarComments = $mediaRules[0]->getComments();
         self::assertCount(1, $fooBarComments);
         self::assertSame('* Number 10 *', $fooBarComments[0]->getComment());
 
         // Media -> declaration -> rule.
+        self::assertInstanceOf(RuleSet::class, $mediaRules[0]);
         $fooBarRules = $mediaRules[0]->getRules();
         $fooBarChildComments = $fooBarRules[0]->getComments();
         self::assertCount(1, $fooBarChildComments);
@@ -1083,6 +1113,7 @@ public function flatCommentExtractingOneComment(): void
         $document = $parser->parse();
 
         $contents = $document->getContents();
+        self::assertInstanceOf(RuleSet::class, $contents[0]);
         $divRules = $contents[0]->getRules();
         $comments = $divRules[0]->getComments();
 
@@ -1099,6 +1130,7 @@ public function flatCommentExtractingTwoConjoinedCommentsForOneRule(): void
         $document = $parser->parse();
 
         $contents = $document->getContents();
+        self::assertInstanceOf(RuleSet::class, $contents[0]);
         $divRules = $contents[0]->getRules();
         $comments = $divRules[0]->getComments();
 
@@ -1116,6 +1148,7 @@ public function flatCommentExtractingTwoSpaceSeparatedCommentsForOneRule(): void
         $document = $parser->parse();
 
         $contents = $document->getContents();
+        self::assertInstanceOf(RuleSet::class, $contents[0]);
         $divRules = $contents[0]->getRules();
         $comments = $divRules[0]->getComments();
 
@@ -1133,6 +1166,7 @@ public function flatCommentExtractingCommentsForTwoRules(): void
         $document = $parser->parse();
 
         $contents = $document->getContents();
+        self::assertInstanceOf(RuleSet::class, $contents[0]);
         $divRules = $contents[0]->getRules();
         $rule1Comments = $divRules[0]->getComments();
         $rule2Comments = $divRules[1]->getComments();
@@ -1151,6 +1185,7 @@ public function topLevelCommentExtracting(): void
         $parser = new Parser('/*Find Me!*/div {left:10px; text-align:left;}');
         $document = $parser->parse();
         $contents = $document->getContents();
+        self::assertInstanceOf(Commentable::class, $contents[0]);
         $comments = $contents[0]->getComments();
         self::assertCount(1, $comments);
         self::assertSame('Find Me!', $comments[0]->getComment());
@@ -1216,6 +1251,7 @@ public function escapedSpecialCaseTokens(): void
     {
         $document = self::parsedStructureForFile('escaped-tokens');
         $contents = $document->getContents();
+        self::assertInstanceOf(RuleSet::class, $contents[0]);
         $rules = $contents[0]->getRules();
         $urlRule = $rules[0];
         $calcRule = $rules[1];