Skip to content

Add Type::getUnionedTypes(), deprecate TypeUtils::flattenTypes()#1922

Closed
herndlm wants to merge 1 commit intophpstan:1.9.xfrom
herndlm:get-unioned-types
Closed

Add Type::getUnionedTypes(), deprecate TypeUtils::flattenTypes()#1922
herndlm wants to merge 1 commit intophpstan:1.9.xfrom
herndlm:get-unioned-types

Conversation

@herndlm
Copy link
Copy Markdown
Contributor

@herndlm herndlm commented Oct 26, 2022

Just an idea I had already a while ago and wanted to discuss. The essence of flattenTypes() is to get the inner types of a union or pass-through the current type in case it's not a union, which is how I came to that name.

I dislike a bit that it's another very specific thing again. But I didn't come up with something smarter / more generic.

And there is still a problem with the regression test for phpstan/phpstan#6379 in NonexistentOffsetInArrayDimFetchCheck which I couldn't figure out yet. But I also don't really understand the narrowed types, see https://phpstan.org/r/788351dc-d1f1-4a17-a6a5-f0120a32222e. The error that is reported is 16: Offset 'c' does not exist on array{cr?: string, c?: string}&non-empty-array..

@herndlm herndlm force-pushed the get-unioned-types branch 3 times, most recently from 3d47633 to 1d5bd62 Compare October 26, 2022 10:14
@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Oct 26, 2022

the 11 failing "normal" tests are expected, it's the regression test that I mentioned in the description

the 4 phpstan failures seem to be related to recent changes on 1.9.x

and the interesting part I wanted to check are the 2 integration failures. I think both of them are fine.
e.g. https://github.com/sebastianbergmann/phpunit/blob/93d4bf4c37aec6384bb9e5d390d9049a463a7256/src/Framework/TestCase.php has 4 ReflectionException dead catches instead of 2, because in all cases the class is existing. but I have no clue how and why this would show up here now 😅
and the PMMP failures are related to the LevelsIntegrationTest that was needed here too but I think is correct. the offset can be non-existent in the union dim. the stringOffsetAccess levels test also shows that those were already reported as potential invalid keys.

@ondrejmirtes
Copy link
Copy Markdown
Member

I don't love this very much, I spoke against dressing flattenTypes as a new method here #1687 (comment). My problem with that is that it doesn't describe any specific use-case, and can be wrongly used or implemented. I'm for methods that feel more like a specific use-case.

BTW we don't have to drain all array use cases before moving on, I'd rather address wrong usages that are most common, like:

  • instanceof TypeWithClassName
  • TypeUtils::getDirectClassNames()

To me it feels like these usages should be replaced with Type::getObjectClassNames(): array

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Oct 26, 2022

ok, got you. I started having this obsession a while ago, where the final goal is to get rid of ConstantArrayType::getAllArrays() 😅

the mentioned getDirectClassNames() cleanup sounds like a good one, yeah 👍

@herndlm herndlm closed this Oct 26, 2022
@herndlm herndlm deleted the get-unioned-types branch October 26, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants