From 08c79142fb41973db2b55e845e9c10428b39bec9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Sat, 19 Feb 2022 04:25:14 +0100 Subject: [PATCH] ImmutableRange: Add @property annotations for magic props Phan can analyze them now and reports some issues with types. * Add some assertions on types where we're sure that we're using an Element or non-null, but Phan can't prove it * Fix incorrect type hints on getFullyCoveredSiblings() and getCoveredSiblings(), luckily it was harmless Change-Id: I8cc12450378efa7434c4d66882378b715edd4a70 --- includes/CommentFormatter.php | 9 +++++++-- includes/CommentModifier.php | 2 ++ includes/CommentParser.php | 3 +++ includes/CommentUtils.php | 4 ++-- includes/HeadingItem.php | 4 ++++ includes/ImmutableRange.php | 7 +++++++ modules/utils.js | 4 ++-- 7 files changed, 27 insertions(+), 6 deletions(-) diff --git a/includes/CommentFormatter.php b/includes/CommentFormatter.php index 2b09ab7e5..76db5d781 100644 --- a/includes/CommentFormatter.php +++ b/includes/CommentFormatter.php @@ -11,6 +11,8 @@ use ParserOutput; use Throwable; use Title; use WebRequest; +use Wikimedia\Assert\Assert; +use Wikimedia\Parsoid\DOM\Element; use Wikimedia\Parsoid\Utils\DOMCompat; use Wikimedia\Parsoid\Utils\DOMUtils; use Wikimedia\Parsoid\Wt2Html\XMLSerializer; @@ -132,9 +134,12 @@ class CommentFormatter { $itemJSON = json_encode( $itemData ); if ( $threadItem instanceof HeadingItem ) { - $threadItem->getRange()->endContainer->setAttribute( 'data-mw-comment', $itemJSON ); + // , or in Parsoid HTML + $headline = $threadItem->getRange()->endContainer; + Assert::precondition( $headline instanceof Element, 'HeadingItem refers to an element node' ); + $headline->setAttribute( 'data-mw-comment', $itemJSON ); if ( $threadItem->isSubscribable() ) { - $headingNode = CommentUtils::closestElement( $threadItem->getRange()->endContainer, [ 'h2' ] ); + $headingNode = CommentUtils::closestElement( $headline, [ 'h2' ] ); if ( $headingNode ) { DOMCompat::getClassList( $headingNode )->add( 'ext-discussiontools-init-section' ); diff --git a/includes/CommentModifier.php b/includes/CommentModifier.php index eec92a844..ae2502ad0 100644 --- a/includes/CommentModifier.php +++ b/includes/CommentModifier.php @@ -4,6 +4,7 @@ namespace MediaWiki\Extension\DiscussionTools; use MediaWiki\MediaWikiServices; use MWException; +use Wikimedia\Assert\Assert; use Wikimedia\Parsoid\DOM\Document; use Wikimedia\Parsoid\DOM\DocumentFragment; use Wikimedia\Parsoid\DOM\Element; @@ -151,6 +152,7 @@ class CommentModifier { $target = $target->parentNode; } + Assert::precondition( $target !== null, 'We have not stepped outside the document' ); // Instead of just using $curComment->getLevel(), consider indentation of lists within the // comment (T252702) $curLevel = CommentUtils::getIndentLevel( $target, $curComment->getRootNode() ) + 1; diff --git a/includes/CommentParser.php b/includes/CommentParser.php index 15846ce35..9688f2dd0 100644 --- a/includes/CommentParser.php +++ b/includes/CommentParser.php @@ -11,6 +11,7 @@ use Language; use MediaWiki\Languages\LanguageConverterFactory; use MWException; use Title; +use Wikimedia\Assert\Assert; use Wikimedia\IPUtils; use Wikimedia\Parsoid\DOM\Element; use Wikimedia\Parsoid\DOM\Node; @@ -930,6 +931,7 @@ class CommentParser { } elseif ( $threadItem instanceof HeadingItem ) { // , or in Parsoid HTML $headline = $threadItem->getRange()->startContainer; + Assert::precondition( $headline instanceof Element, 'HeadingItem refers to an element node' ); $id = 'h-' . $this->truncateForId( $headline->getAttribute( 'id' ) ?? '' ); } elseif ( $threadItem instanceof CommentItem ) { $id = 'c-' . $this->truncateForId( str_replace( ' ', '_', $threadItem->getAuthor() ) ) . @@ -944,6 +946,7 @@ class CommentParser { if ( $threadItemParent instanceof HeadingItem && !$threadItemParent->isPlaceholderHeading() ) { // , or in Parsoid HTML $headline = $threadItemParent->getRange()->startContainer; + Assert::precondition( $headline instanceof Element, 'HeadingItem refers to an element node' ); $id .= '-' . $this->truncateForId( $headline->getAttribute( 'id' ) ?? '' ); } elseif ( $threadItemParent instanceof CommentItem ) { $id .= '-' . $this->truncateForId( str_replace( ' ', '_', $threadItemParent->getAuthor() ) ) . diff --git a/includes/CommentUtils.php b/includes/CommentUtils.php index f49457581..112c51b0d 100644 --- a/includes/CommentUtils.php +++ b/includes/CommentUtils.php @@ -349,7 +349,7 @@ class CommentUtils { * Get an array of sibling nodes that contain parts of the given range. * * @param ImmutableRange $range - * @return Element[] + * @return Node[] */ public static function getCoveredSiblings( ImmutableRange $range ): array { $ancestor = $range->commonAncestorContainer; @@ -385,7 +385,7 @@ class CommentUtils { * Get the nodes (if any) that contain the given thread item, and nothing else. * * @param ThreadItem $item - * @return Element[]|null + * @return Node[]|null */ public static function getFullyCoveredSiblings( ThreadItem $item ): ?array { $siblings = self::getCoveredSiblings( $item->getRange() ); diff --git a/includes/HeadingItem.php b/includes/HeadingItem.php index 28454f092..37df2775f 100644 --- a/includes/HeadingItem.php +++ b/includes/HeadingItem.php @@ -2,6 +2,9 @@ namespace MediaWiki\Extension\DiscussionTools; +use Wikimedia\Assert\Assert; +use Wikimedia\Parsoid\DOM\Element; + class HeadingItem extends ThreadItem { private $placeholderHeading = false; private $headingLevel; @@ -40,6 +43,7 @@ class HeadingItem extends ThreadItem { if ( !$this->isPlaceholderHeading() ) { // , or in Parsoid HTML $headline = $this->getRange()->startContainer; + Assert::precondition( $headline instanceof Element, 'HeadingItem refers to an element node' ); $id = $headline->getAttribute( 'id' ); if ( $id ) { // Replace underscores with spaces to undo Sanitizer::escapeIdInternal(). diff --git a/includes/ImmutableRange.php b/includes/ImmutableRange.php index fd56edc00..6c079f55d 100644 --- a/includes/ImmutableRange.php +++ b/includes/ImmutableRange.php @@ -18,6 +18,13 @@ use Wikimedia\Parsoid\DOM\Text; * which is lazy evaluated. * * setStart and setEnd are still available but return a cloned range. + * + * @property bool $collapsed + * @property Node $commonAncestorContainer + * @property Node $endContainer + * @property int $endOffset + * @property Node $startContainer + * @property int $startOffset */ class ImmutableRange { private $mCommonAncestorContainer; diff --git a/modules/utils.js b/modules/utils.js index d910a8d7c..d5d765d5e 100644 --- a/modules/utils.js +++ b/modules/utils.js @@ -302,7 +302,7 @@ function getIndentLevel( node, rootNode ) { * Get an array of sibling nodes that contain parts of the given range. * * @param {Range} range - * @return {HTMLElement[]} + * @return {Node[]} */ function getCoveredSiblings( range ) { var ancestor = range.commonAncestorContainer; @@ -336,7 +336,7 @@ function getCoveredSiblings( range ) { * Get the nodes (if any) that contain the given thread item, and nothing else. * * @param {ThreadItem} item Thread item - * @return {HTMLElement[]|null} + * @return {Node[]|null} */ function getFullyCoveredSiblings( item ) { var siblings = getCoveredSiblings( item.getNativeRange() );