Use isArray, isConstantArray instead of instanceof in TypeCombinartor::union #2118

rajyan · 2022-12-16T00:31:07Z

Refactored union of ArrayTypes by extracting processArrayAccessoryTypes as a private function.
Also, changed to use isArray isConstantArray instead of instanceof ArrayType.

I think we can apply the bucket idea of phpstan/phpstan#8499 if we filter ArrayTypes, ConstantIntegerTypes|IntegerRangeType and union them before creating buckets.

ondrejmirtes · 2022-12-16T08:28:14Z

src/Type/Accessory/AccessoryArrayListType.php

@@ -239,7 +239,7 @@ public function getLastIterableValueType(): Type

 	public function isArray(): TrinaryLogic
 	{
-		return TrinaryLogic::createYes();
+		return TrinaryLogic::createMaybe();


I haven't checked the rest of this PR but this is certainly wrong. If something is a list then we know it's an array...

I understand you're changing this so that some other algorithm results in an outcome we want, but the typesystem layer always has to tell the truth, otherwise something else breaks...

I haven't checked the rest of this PR but this is certainly wrong. If something is a list then we know it's an array...

I thought that AccessoryType themselves are not isArray, they have to be always intersected with an array, but looking at the failing test, this assumption seems false as you are saying...

I can solve this PR without this change, I just wanted to check what fails with this :) (at least the tests didn't fail, maybe we need some regression tests)

I'm likely to accept whatever you come up with, but make sure to do some benchmarks too :) It's really easy to think you've sped things up, but in reality you might have slowed them down :) Thanks.

This reverts commit 984a5d3.

…scribe(VerbosityLevel::cache())` and they should be same (except HasOffsetValueType)

rajyan · 2022-12-17T13:05:50Z

I tried to just refactor and keep the original behavior, but there seems to be a slight difference somewhere...

rajyan · 2022-12-17T13:11:02Z

╭─ ~/Contribution/phpstan-src  refactor-union *2 !1 ···································· 1 ✘  4s  22:01:50 
╰─ hyperfine 'bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php'
Benchmark 1: bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php
  Time (mean ± σ):      9.168 s ±  0.106 s    [User: 8.924 s, System: 0.195 s]
  Range (min … max):    9.056 s …  9.407 s    10 runs
 
╭─ ~/Contribution/phpstan-src  refactor-union *2 ·············································· ✔  22:06:01 
╰─ git switch 1.9.x
Switched to branch '1.9.x'
Your branch is up to date with 'origin/1.9.x'.
╭─ ~/Contribution/phpstan-src  1.9.x *2 ······················································· ✔  22:06:04 
╰─ hyperfine 'bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php'
Benchmark 1: bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php
  Time (mean ± σ):      9.754 s ±  0.070 s    [User: 9.481 s, System: 0.214 s]
  Range (min … max):    9.696 s …  9.893 s    10 runs

I believe this refactor should have a slight performance improvement as I expected, because number of union calls has been decreased.

rajyan · 2022-12-19T00:59:29Z

╭─ ~/Contribution/phpstan-src  refactor-union *2 ··· INT ✘  30s  09:53:14 
╰─ hyperfine 'bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php'
Benchmark 1: bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php
  Time (mean ± σ):      9.121 s ±  0.069 s    [User: 8.884 s, System: 0.190 s]
  Range (min … max):    9.055 s …  9.259 s    10 runs
 
╭─ ~/Contribution/phpstan-src  refactor-union *2 ···· ✔  1m 31s  09:54:46 
╰─ git switch 1.9.x
Switched to branch '1.9.x'
Your branch is up to date with 'origin/1.9.x'.
╭─ ~/Contribution/phpstan-src  1.9.x *2 ······················ ✔  09:54:53 
╰─ hyperfine 'bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php'
Benchmark 1: bin/phpstan analyse src/Analyser/NodeScopeResolver.php src/Analyser/MutatingScope.php
  Time (mean ± σ):      9.160 s ±  0.061 s    [User: 8.921 s, System: 0.185 s]
  Range (min … max):    9.067 s …  9.271 s    10 runs
 
  Warning: The first benchmarking run for this command was significantly slower than the rest (9.271 s). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.

This is the final performance comparison.
This PR is preparation for #2078 looks like there isn't a significant performance improvement.

phpstan-bot · 2022-12-19T01:00:27Z

This pull request has been marked as ready for review.

rajyan · 2022-12-19T02:53:32Z

src/Type/TypeCombinator.php

-		$arrayTypes = $arrayTypes;
-
-		$commonArrayAccessoryTypesKeys = [];
-		if (count($arrayAccessoryTypes) > 1) {
-			$commonArrayAccessoryTypesKeys = array_keys(array_intersect_key(...$arrayAccessoryTypes));
-		} elseif (count($arrayAccessoryTypes) > 0) {
-			$commonArrayAccessoryTypesKeys = array_keys($arrayAccessoryTypes[0]);
-		}
-
-		$arrayAccessoryTypesToProcess = [];
-		foreach ($commonArrayAccessoryTypesKeys as $commonKey) {
-			$typesToUnion = [];
-			foreach ($arrayAccessoryTypes as $array) {
-				foreach ($array[$commonKey] as $arrayAccessoryType) {
-					$typesToUnion[] = $arrayAccessoryType;
-				}
-			}
-			$arrayAccessoryTypesToProcess[] = self::union(...$typesToUnion);
-		}


this logic is replaced by count($arrayType) === count($accessoryType[$key]) in
https://github.com/phpstan/phpstan-src/pull/2118/files#diff-fef252fe8071200a31b3d282d1442135d62297098c9b20fd731f3888398ee973R549-R562

also, there is no need to union accessoryTypes except HasOffsetValueType, because the types are all the same.

ondrejmirtes · 2022-12-19T11:34:45Z

Thank you!

rajyan marked this pull request as draft December 16, 2022 00:35

rajyan force-pushed the refactor-union branch from ab253c8 to b39adf9 Compare December 16, 2022 00:55

ondrejmirtes reviewed Dec 16, 2022

View reviewed changes

rajyan added 6 commits December 17, 2022 21:33

move IterableType union logic to compareTypesInUnion

46ee018

AccessoryTypes themselves are not isArray->yes

3558d3d

refactor union of array types by using isArray, isConstantArray

3e94bd8

cs-fix

e734fb4

Revert "AccessoryTypes themselves are not isArray->yes"

116a2be

This reverts commit 984a5d3.

no need to union AccessoryTypes because classified by `$innerType->de…

7581b9e

…scribe(VerbosityLevel::cache())` and they should be same (except HasOffsetValueType)

rajyan force-pushed the refactor-union branch from 8900cef to 99ec877 Compare December 17, 2022 13:01

micro optimization

b4719b8

rajyan force-pushed the refactor-union branch from 99ec877 to b4719b8 Compare December 17, 2022 13:05

rajyan added 2 commits December 19, 2022 09:19

add failing tests for wrong union result type

b611f52

fix to use isConstantArray in reduceArrays too

d0626fb

rajyan force-pushed the refactor-union branch from 3ed21be to d0626fb Compare December 19, 2022 00:51

rajyan changed the title ~~Refactor union~~ Use isArray, isConstantArray instead of instanceof in TypeCombinartor::union Dec 19, 2022

rajyan marked this pull request as ready for review December 19, 2022 01:00

rajyan commented Dec 19, 2022

View reviewed changes

ondrejmirtes merged commit 679538e into phpstan:1.9.x Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use isArray, isConstantArray instead of instanceof in TypeCombinartor::union #2118

Use isArray, isConstantArray instead of instanceof in TypeCombinartor::union #2118

rajyan commented Dec 16, 2022 •

edited

ondrejmirtes Dec 16, 2022

ondrejmirtes Dec 16, 2022

rajyan Dec 16, 2022

ondrejmirtes Dec 16, 2022

rajyan commented Dec 17, 2022 •

edited

rajyan commented Dec 17, 2022 •

edited

rajyan commented Dec 19, 2022 •

edited

phpstan-bot commented Dec 19, 2022

rajyan Dec 19, 2022 •

edited

ondrejmirtes commented Dec 19, 2022

Use isArray, isConstantArray instead of instanceof in TypeCombinartor::union #2118

Use isArray, isConstantArray instead of instanceof in TypeCombinartor::union #2118

Conversation

rajyan commented Dec 16, 2022 • edited

ondrejmirtes Dec 16, 2022

Choose a reason for hiding this comment

ondrejmirtes Dec 16, 2022

Choose a reason for hiding this comment

rajyan Dec 16, 2022

Choose a reason for hiding this comment

ondrejmirtes Dec 16, 2022

Choose a reason for hiding this comment

rajyan commented Dec 17, 2022 • edited

rajyan commented Dec 17, 2022 • edited

rajyan commented Dec 19, 2022 • edited

phpstan-bot commented Dec 19, 2022

rajyan Dec 19, 2022 • edited

Choose a reason for hiding this comment

ondrejmirtes commented Dec 19, 2022

rajyan commented Dec 16, 2022 •

edited

rajyan commented Dec 17, 2022 •

edited

rajyan commented Dec 17, 2022 •

edited

rajyan commented Dec 19, 2022 •

edited

rajyan Dec 19, 2022 •

edited