Add built-in return types #5367

malberts · 2022-12-22T06:12:16Z

Done with Rector. This required multiple runs and some manual fixes where SMW was too ambiguous. My understanding is the more return types you have, the better it is able to infer additional missing types.

There are still cases where built-in types are missing, however in the current state Rector no longer adds anything for me, even after clearing Rector's cache. That might be due to ambiguity, or I suspect I need to use additional Rector rules, but I will try them in follow-up PRs.

The array return type refactoring adds comments documenting the array contents. I did not manually touch or review any of those additions.

This PR can be squashed. I did separate commits to make the reruns/manual fixes clearer for myself.

Rector config:

<?php

declare( strict_types=1 );

use Rector\Config\RectorConfig;
use Rector\CodeQuality\Rector\ClassMethod\ReturnTypeFromStrictScalarReturnExprRector;
use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictBoolReturnExprRector;
use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictConstantReturnRector;
use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictNativeCallRector;
use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictNewArrayRector;
use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictTypedCallRector;
use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictTypedPropertyRector;

return static function ( RectorConfig $rectorConfig ): void {
	$rectorConfig->indent( "\t", 1);

	$rectorConfig->paths( [
		__DIR__ . '/includes',
		__DIR__ . '/src',
	] );

	$rectorConfig->skip( [
		//
		ReturnTypeFromStrictScalarReturnExprRector::class => [
			// getServiceLinkParams()
			__DIR__ . '/includes/datavalues/SMW_DataValue.php',
			// getUnit()
			__DIR__ . '/includes/datavalues/SMW_DV_Number.php',
		]
	] );

	$rectorConfig->rules( [
		ReturnTypeFromStrictScalarReturnExprRector::class,
		ReturnTypeFromStrictBoolReturnExprRector::class,
		ReturnTypeFromStrictConstantReturnRector::class,
		ReturnTypeFromStrictNativeCallRector::class,
		ReturnTypeFromStrictNewArrayRector::class,
		ReturnTypeFromStrictTypedCallRector::class,
		ReturnTypeFromStrictTypedPropertyRector::class,
	] );
};

malberts · 2022-12-22T07:24:21Z

includes/datavalues/SMW_DV_Number.php

 	 */
-	public function getUnit(): string {
+	public function getUnit() {


This is a manual fix where Rector didn't understand that a subclass (SMWQuantityValue) can return bool here.

I did not review the code to see if it's just a theorerical issue (i.e. perhaps it's bool initially but always changes to a string). The class member m_mainunit is typed as string|bool.

malberts · 2022-12-22T07:24:29Z

includes/datavalues/SMW_DataValue.php

 	 */
-	protected function getServiceLinkParams(): bool {
+	protected function getServiceLinkParams() {


This is a manual fix where Rector didn't understand that the subclasses return arrays.

malberts · 2022-12-22T07:34:04Z

~~Eh it messed up some comment indentations. Looks like you need to configure that first.~~

Fixed.

malberts · 2022-12-22T08:25:16Z

src/MediaWiki/Page/ListBuilder.php

@@ -164,7 +164,10 @@ public function getColumnList( array $dataItems, $colsThreshold = 10 ) {
 		return $htmlColumns->getHtml();
 	}

-	private function buildList( $dataItems ) {
+	/**
+	 * @return array<int|string, mixed[]>


This is an example of the automatically added array type. If this kind of documentation is pointless (albeit required for the CS tools) then I can rather remove these now. If somebody looks at this code later and comes up with a better type we can add it then.

This one does not seem super helpful. I'm also concerned correctness. If Rector is taking info from existing info in docs, then it might amplify mistakes in those docs. I do not trust type docs in SMW as I've seen them be wrong multiple times in the past.

malberts · 2022-12-22T08:43:21Z

src/SQLStore/Lookup/TableStatisticsLookup.php

@@ -47,7 +47,10 @@ public function get( $key ) {
 		return $this->{$key}( $this->store->getConnection( 'mw.db' ) );
 	}

-	private function loadFromDB( $connection ) {
+	/**
+	 * @return array{total_row_count: mixed, last_id: mixed, duplicate_count: int, rows: array{rev_count: mixed, smw_namespace_group_by_count: mixed, smw_iw: array{delete_count: mixed, redirect_count: mixed}, smw_proptable_hash: array{query_match_count: mixed, query_null_count: mixed}}}[]|array{total_row_count: mixed, rows: array{active_links_count: mixed, invalid_links_count: mixed, unassigned_count: mixed}}[]|array{total_row_count: mixed, unique_terms_occurrence_in_percent: int|float, rows: array{blob_field_null_row_count: mixed, terms_occurrence: array{single_occurrence_total_count: mixed, multi_occurrence_total_count: mixed}}}[]|array{query_time: float, snapshot_date: string}[]


Possibly yes

malberts · 2022-12-22T08:50:15Z

src/SQLStore/TableBuilder/MySQLTableBuilder.php

@@ -143,7 +143,10 @@ protected function doUpdateTable( $tableName, array $attributes = null ) {
 		}
 	}

-	private function getCurrentFields( $tableName ) {
+	/**
+	 * @return string[]|string[][]


This is wrong, but is strictly correct because $row->Type could be an array and str_replace can run on an array, hence why it thinks you can have 2D array.

JeroenDeDauw · 2022-12-22T13:42:23Z

includes/datavalues/SMW_DataValue.php

@@ -759,7 +759,7 @@ public function getHash() {
 	 *
 	 * @return boolean
 	 */
-	public function isNumeric() {
+	public function isNumeric(): bool {


Return type narrowing to base classes or interfaces that are extended in other extensions require a major release. For this particular class, possibly Maps is the only affected extension, with its Geo DataValue.

Changes to ResultPrinter will have a wider impact

We can always make a pragmatic exception to the rule, but would need to asses the impact of doing so. Or we could bump to 5.x.

Either way, probably good to first do #5366 before merging potentially disruptive changes.

What do you suggest? Should we aim for a new major version in the short term, or is adding types like this too much effort? Otherwise I'll rather focus on missing types throwing deprecation notices in PHP 8.1 (e.g. #5370). But I suppose there is still a chance that by adding a missing type there we might still cause narrowing. I'm not familiar with all the downstream extensions, so that makes it a bit difficult to test.

edit: I missed your second comment. Yes, doing #5366 first sounds like a good idea. If we end up bumping to 5.x we should probably also review the minimum PHP version.

We should definitely throw out 7.3, maybe also 7.4.

New major version in the short term seems fine to me, but would still do a patch/minor release first.

malberts marked this pull request as draft December 22, 2022 06:12

malberts force-pushed the scalar-types branch from a8c34b5 to d3794a5 Compare December 22, 2022 06:27

malberts added 2 commits December 22, 2022 08:30

Add built-in return types

1fdb321

Fix ambiguous types

73ec887

malberts force-pushed the scalar-types branch from d3794a5 to 73ec887 Compare December 22, 2022 06:30

malberts changed the title ~~Add scalar return types~~ Add built-in return types Dec 22, 2022

malberts added 4 commits December 22, 2022 09:02

Add built-in return types

cd615fd

Fix return type

3643963

Add built-in return types

6cebdd4

Add built-in return types

8374110

malberts marked this pull request as ready for review December 22, 2022 07:21

malberts commented Dec 22, 2022

View reviewed changes

malberts marked this pull request as draft December 22, 2022 07:33

malberts added 3 commits December 22, 2022 09:42

Fix comment indents

0116197

Fix function indents

60ece17

Fix function indents

e352033

malberts commented Dec 22, 2022

View reviewed changes

Fix indents

736c19d

malberts force-pushed the scalar-types branch from 4f32543 to 736c19d Compare December 22, 2022 08:30

malberts commented Dec 22, 2022

View reviewed changes

malberts marked this pull request as ready for review December 22, 2022 09:08

malberts mentioned this pull request Dec 22, 2022

Fix PHP 8.1 deprecations (during test execution) #5370

Closed

JeroenDeDauw reviewed Dec 22, 2022

View reviewed changes

malberts marked this pull request as draft December 23, 2022 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add built-in return types #5367

Add built-in return types #5367

malberts commented Dec 22, 2022 •

edited

malberts Dec 22, 2022 •

edited

malberts Dec 22, 2022

malberts commented Dec 22, 2022 •

edited

malberts Dec 22, 2022

JeroenDeDauw Dec 22, 2022

malberts Dec 22, 2022

JeroenDeDauw Dec 22, 2022

malberts Dec 22, 2022

JeroenDeDauw Dec 22, 2022

JeroenDeDauw Dec 22, 2022

malberts Dec 22, 2022 •

edited

JeroenDeDauw Dec 22, 2022

Add built-in return types #5367

Are you sure you want to change the base?

Add built-in return types #5367

Conversation

malberts commented Dec 22, 2022 • edited

malberts Dec 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malberts commented Dec 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malberts Dec 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malberts commented Dec 22, 2022 •

edited

malberts Dec 22, 2022 •

edited

malberts commented Dec 22, 2022 •

edited

malberts Dec 22, 2022 •

edited