array_merge lost non-empty-string keys type #1135

staabm · 2022-03-28T17:10:22Z

suggestion for a fix of bug #6927.

I am wondering why the generalize call was in the ArrayMergeFunctionDynamicReturnTypeExtension

.. the only thing I could think of are "cosmetic" reasons for error messages, since the types we get when dropping the generalize call seem to be more precise and still valid? lets see what CI thinks about it.

the generalize call was added a long time ago with 002aa73

closes phpstan/phpstan#6927

staabm · 2022-03-28T17:17:01Z

unrelated: there is a interessting test-case, I tried to add.

	/**
	 * @param array<numeric-string&non-empty-string, string> $params1
	 * @param array<numeric-string, string> $params2
	 */
	function foo5(array $params1, array $params2): void
	{
		$params2 = array_merge($params1, $params2);

		assertType('array<numeric-string, string>', $params2);
	}

it errors with

There was 1 failure:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser/data/bug-6927.php:61" ('type', 'C:\dvl\Workspace\phpstan-src-...27.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\ArrayType Object (...), 61)
Expected type array<numeric-string, string>, got type array<int, string> in C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser/data/bug-6927.php on line 61.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'array<numeric-string, string>'
+'array<int, string>'

C:\dvl\Workspace\phpstan-src-staabm\src\Testing\TypeInferenceTestCase.php:97
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\NodeScopeResolverTest.php:846

which is a different bug IMO.. but interessting nevertheless

edit: opened a new issue for that

ondrejmirtes · 2022-03-28T19:08:56Z

I agree, it shouldn't be generalized.

staabm · 2022-03-30T12:39:20Z

the remaining error

Error: Property Composer\Util\Http\CurlDownloader::$jobs (
array<array{url: string, origin: string, attributes: array{retryAuthFailure: bool, redirects: int, retries: int, storeAuth: bool}, options: array, progress: array, curlHandle: resource, filename: string|false, headerHandle: resource, ...}>) 
does not accept 
non-empty-array<array{url: string, origin: string, attributes: array{retryAuthFailure: bool, redirects: int, retries: int, storeAuth: bool}, options: array, progress: array, curlHandle: CurlHandle|resource, filename: string|false|null, headerHandle: resource, ...}>.

seems legit and unrelated, because resource and CurlHandle|resource, and also string|false and string|false|null do not match.
see https://github.com/composer/composer/blob/3ae111140facdba8ae82adcd1085e4adfc7d715c/src/Composer/Util/Http/CurlDownloader.php#L261-L273

the code seems not to be dependent on array_merge which is the one we change with this PR.

staabm · 2022-03-30T12:42:16Z

tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php

@@ -4707,31 +4707,31 @@ public function dataArrayFunctions(): array
 				'array_values($generalStringKeys)',
 			],
 			[
-				'non-empty-array<int|(literal-string&non-empty-string), stdClass>',
+				'array{foo: stdClass, 1: stdClass}',


changed existing tests, to more specific types - as the PR gets better results in these cases.

staabm · 2022-03-30T12:50:16Z

tests/PHPStan/Analyser/data/bug-6927.php

+	 * @param array{return: int, stdout: string, stderr: string} $params1
+	 * @param array{return: int, stdout?: string, stderr?: string} $params2


this test is inspired by a scenario which failed with a previous version of this PR within the composer code-base

src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php

ondrejmirtes · 2022-03-30T14:08:22Z

src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php

+			$newArrayBuilder = ConstantArrayTypeBuilder::createEmpty();
+			foreach ($argTypes as $argType) {
+				if (!$argType instanceof ConstantArrayType) {
+					throw new ShouldNotHappenException();


You could push only ConstantArrayType into $argTypes and this condition would not be necessary :)

but then I couldn't re-use the $artTypes for the 2nd-foreach loop..?

or I have all non-constant-array-args to be resolved twice via scope->getType
or alternatively I need 2 arrays to hold the different args per type

ondrejmirtes · 2022-03-30T14:57:24Z

Thank you, let's try this!

ondrejmirtes · 2022-03-30T21:43:30Z

The same thing that happened last time someone tried to do this happened again :( #641 (comment)

This version is about 30 % slower on Slevomat's codebase than previous commit. I'm gonna revert this. Can you please send a new PR that just removes the generalization to see if it fixes the related issues? https://github.com/phpstan/phpstan-src/pull/1135/files#diff-235402991c12d4f1091a88a9c19ed82c38749ed4e4dce1322474ed6c6d7fdcc8L47-R92

ondrejmirtes · 2022-03-30T21:44:59Z

Oh, previous commit is also slow, I overlooked it. Well, we have a rabbit hole to go through.

ondrejmirtes · 2022-03-30T21:49:57Z

Alright, it's the same speed as the last stable version, we can release this as it is. (It might have something to do I'm on battery right now, I'm used to seeing faster numbers though.)

staabm changed the title ~~stab at bug 6927~~ array_merge lost non-empty-string keys type Mar 28, 2022

staabm mentioned this pull request Mar 30, 2022

fix phpdoc type in ProcessExecutorMock composer/composer#10669

Merged

staabm force-pushed the bug6927 branch from 67ca40f to 11ac6b3 Compare March 30, 2022 12:20

staabm mentioned this pull request Mar 30, 2022

array_merge() and numeric-string keys phpstan/phpstan#6945

Closed

staabm marked this pull request as ready for review March 30, 2022 12:39

staabm commented Mar 30, 2022

View reviewed changes

ondrejmirtes requested changes Mar 30, 2022

View reviewed changes

src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Mar 30, 2022

View reviewed changes

clxmstaab force-pushed the bug6927 branch from 3402234 to 6d4ff40 Compare March 30, 2022 14:09

clxmstaab added 7 commits March 30, 2022 16:14

stab at bug 6927

59b7244

cs

bfe09fc

another testcase

1b34d8e

added failling testcase

e2e637e

fix test

800beb9

fix

05d7fe3

cleanup

98a618e

clxmstaab force-pushed the bug6927 branch from 6d4ff40 to 98a618e Compare March 30, 2022 14:14

ondrejmirtes merged commit 565248d into phpstan:1.5.x Mar 30, 2022

staabm deleted the bug6927 branch March 30, 2022 14:58

This was referenced Mar 30, 2022

array_merge of known shapes loses type information phpstan/phpstan#5846

Closed

False positive with array_merge phpstan/phpstan#2567

Closed

ondrejmirtes mentioned this pull request Mar 30, 2022

array_merge lost non-empty-string keys type phpstan/phpstan#6927

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

array_merge lost non-empty-string keys type #1135

array_merge lost non-empty-string keys type #1135

staabm commented Mar 28, 2022 •

edited

staabm commented Mar 28, 2022 •

edited

ondrejmirtes commented Mar 28, 2022

staabm commented Mar 30, 2022 •

edited

staabm Mar 30, 2022 •

edited

staabm Mar 30, 2022 •

edited

ondrejmirtes Mar 30, 2022

staabm Mar 30, 2022 •

edited

ondrejmirtes commented Mar 30, 2022

ondrejmirtes commented Mar 30, 2022

ondrejmirtes commented Mar 30, 2022

ondrejmirtes commented Mar 30, 2022

		* @param array{return: int, stdout: string, stderr: string} $params1
		* @param array{return: int, stdout?: string, stderr?: string} $params2

array_merge lost non-empty-string keys type #1135

array_merge lost non-empty-string keys type #1135

Conversation

staabm commented Mar 28, 2022 • edited

staabm commented Mar 28, 2022 • edited

ondrejmirtes commented Mar 28, 2022

staabm commented Mar 30, 2022 • edited

staabm Mar 30, 2022 • edited

Choose a reason for hiding this comment

staabm Mar 30, 2022 • edited

Choose a reason for hiding this comment

ondrejmirtes Mar 30, 2022

Choose a reason for hiding this comment

staabm Mar 30, 2022 • edited

Choose a reason for hiding this comment

ondrejmirtes commented Mar 30, 2022

ondrejmirtes commented Mar 30, 2022

ondrejmirtes commented Mar 30, 2022

ondrejmirtes commented Mar 30, 2022

staabm commented Mar 28, 2022 •

edited

staabm commented Mar 28, 2022 •

edited

staabm commented Mar 30, 2022 •

edited

staabm Mar 30, 2022 •

edited

staabm Mar 30, 2022 •

edited

staabm Mar 30, 2022 •

edited