Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Big array are too much simplified since 1.8.4 #7963

Closed
VincentLanglet opened this issue Sep 7, 2022 · 9 comments
Closed

Big array are too much simplified since 1.8.4 #7963

VincentLanglet opened this issue Sep 7, 2022 · 9 comments

Comments

@VincentLanglet
Copy link
Contributor

VincentLanglet commented Sep 7, 2022

Bug report

Code snippet that reproduces the problem

The following code was working until 1.8.3 version
https://phpstan.org/r/be218157-2818-49fc-9720-983a6de8051f
Now it gives an error

Expected output

I'd like to have no error with this code.

I understood that some changes/simplifications were introduced to array in 1.8.4 because 1.8.3 were too slow
but the following code was fast and were working in 1.8.2 (and 1.8.3) so I assume that under some conditions, it could be still possible to not over-simplified this array.
I dumped the type in 1.8.2 and the array was not simplified.

I see two solutions to my issue

  • Find a way to not break performances and still avoid to simplified the array
  • Simplified the array but preferred array-shape array{} over array, this would be simplified to

Did PHPStan help you today? Did it make you happy in any way?

This tool is awesome.

@xPaw
Copy link

xPaw commented Sep 8, 2022

I'm seeing a similar issue with a big array: https://phpstan.org/r/125b4137-6f1e-46c4-bb2f-7e281c061d45

For this particular array, if I change all the path to an empty string, it no longer errors.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 8, 2022

Someone needs to figure out how to keep PHPStan fast and these types more precise.

@jlherren
Copy link

jlherren commented Sep 9, 2022

Here's a much smaller example to help investigating this more easily, however I'm not 100% sure it's the exact same issue.

https://phpstan.org/r/b6a055a2-00fc-4151-a500-c62506bbdcf9

In this particular example, it should be relatively easy to collapse the union produced by the ternaries back into a single array shape. The two array shapes produced by the two branches of the ternaries only ever differ by one single element. Keeping unions as array{...}|array{...} only makes sense when there are at least two differences, such as to keep information about their correlation (or entanglement if you prefer).

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Sep 18, 2022

Someone needs to figure out how to keep PHPStan fast and these types more precise.

I'm not sure which commit introduce the degradation of the array, if it's phpstan/phpstan-src@0cc87f3 or another.

But if this is related to some constant like https://github.com/phpstan/phpstan-src/blob/8250448ceac7586cf556b6e9c7858b50f6aa50e7/src/Type/Constant/ConstantArrayTypeBuilder.php#L25
a solution could be to make this constant configurable.

"If you want a fast PHPStan, you can use a lower constant, but be aware array are degraded quickly".
"If you want a precise PHPStan, you can use a bigger constant, but be aware it can be slow".

@phpstan-bot
Copy link

phpstan-bot commented Sep 19, 2022

@xPaw After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-9: PHPDoc tag @var for constant Octicons::Data with type array<string, array{width: int, height: int, path: string}> is not subtype of value non-empty-array<'accessibility'|'alert'|'apps'|'archive'|'arrow-both'|'arrow-down'|'arrow-left'|'arrow-right'|'arrow-switch'|'arrow-up'|'beaker'|'bell'|'bell-fill'|'bell-slash'|'blocked'|'bold'|'book'|'bookmark'|'bookmark-slash'|'briefcase'|'broadcast'|'browser'|'bug'|'calendar'|'check'|'check-circle'|'check-circle-fill'|'checklist'|'chevron-down'|'chevron-left'|'chevron-right'|'chevron-up'|'circle'|'circle-slash'|'clock'|'cloud'|'cloud-offline'|'code'|'code-of-conduct'|'code-review'|'code-square'|'codescan'|'codescan-checkmark'|'codespaces'|'columns'|'comment'|'comment-discussion'|'container'|'copilot'|'copilot-error'|'copilot-warning'|'copy'|'cpu'|'credit-card'|'cross-reference'|'dash'|'database'|'dependabot'|'desktop-download'|'device-camera'|'device-camera-video'|'device-desktop'|'device-mobile'|'diamond'|'diff'|'diff-added'|'diff-ignored'|'diff-modified'|'diff-removed'|'diff-renamed'|'dot'|'dot-fill'|'download'|'duplicate'|'ellipsis'|'eye'|'eye-closed'|'facebook'|'feed-discussion'|'feed-forked'|'feed-heart'|'feed-merged'|'feed-person'|'feed-repo'|'feed-rocket'|'feed-star'|'feed-tag'|'feed-trophy'|'file'|'file-added'|'file-badge'|'file-binary'|'file-code'|'file-diff'|'file-directory'|'file-directory-fill'|'file-directory-open…'|'file-moved'|'file-removed'|'file-submodule'|'file-symlink-file'|'file-zip'|'filter'|'flame'|'fold'|'fold-down'|'fold-up'|'gear'|'gift'|'git-branch'|'git-commit'|'git-compare'|'git-merge'|'git-pull-request'|'git-pull-request…'|'git-pull-request…'|'globe'|'grabber'|'graph'|'hash'|'heading'|'heart'|'heart-fill'|'history'|'home'|'horizontal-rule'|'hourglass'|'hubot'|'id-badge'|'image'|'inbox'|'infinity'|'info'|'issue-closed'|'issue-draft'|'issue-opened'|'issue-reopened'|'italic'|'iterations'|'kebab-horizontal'|'key'|'key-asterisk'|'law'|'light-bulb'|'link'|'link-external'|'linux'|'list-ordered'|'list-unordered'|'location'|'lock'|'log'|'logo-gist'|'logo-github'|'macos'|'mail'|'mark-github'|'markdown'|'megaphone'|'mention'|'meter'|'milestone'|'mirror'|'moon'|'mortar-board'|'multi-select'|'mute'|'no-entry'|'north-star'|'note'|'number'|'organization'|'package'|'package-dependencies'|'package-dependents'|'paintbrush'|'paper-airplane'|'paste'|'pencil'|'people'|'person'|'person-add'|'person-fill'|'pin'|'play'|'plug'|'plus'|'plus-circle'|'project'|'pulse'|'question'|'quote'|'reply'|'repo'|'repo-clone'|'repo-deleted'|'repo-forked'|'repo-locked'|'repo-pull'|'repo-push'|'repo-template'|'report'|'rocket'|'rows'|'rss'|'ruby'|'screen-full'|'screen-normal'|'search'|'server'|'share'|'share-android'|'shield'|'shield-check'|'shield-lock'|'shield-x'|'sidebar-collapse'|'sidebar-expand'|'sign-in'|'sign-out'|'single-select'|'skip'|'sliders'|'smiley'|'sort-asc'|'sort-desc'|'square'|'square-fill'|'squirrel'|'stack'|'star'|'star-fill'|'steam'|'steamdb'|'steamdeck'|'steamdeck_playable'|'steamdeck…'|'steamdeck_verified'|'steamworks'|'stop'|'stopwatch'|'strikethrough'|'sun'|'sync'|'tab-external'|'table'|'tag'|'tasklist'|'telescope'|'telescope-fill'|'terminal'|'three-bars'|'thumbsdown'|'thumbsup'|'tools'|'trash'|'triangle-down'|'triangle-left'|'triangle-right'|'triangle-up'|'trophy'|'twitch'|'twitter'|'typography'|'unfold'|'unlock'|'unmute'|'unverified'|'upload'|'verified'|'versions'|'video'|'webhook'|'windows'|'workflow'|'x'|'x-circle'|'x-circle-fill'|'youtube'|'zap', non-empty-array<literal-string&non-falsy-string, int|(literal-string&non-falsy-string)>>.
+No errors

@phpstan-bot
Copy link

phpstan-bot commented Sep 19, 2022

@jlherren After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-19: Dumped type: array{foo: null, bar: null, baz: null, abc: string|null, qwe: string|null, xyz: string|null}|array{foo: null, bar: null, baz: string, abc: string|null, qwe: string|null, xyz: string|null}|array{foo: null, bar: string, baz: null, abc: string|null, qwe: string|null, xyz: string|null}|array{foo: null, bar: string, baz: string, abc: string|null, qwe: string|null, xyz: string|null}|array{foo: string, bar: null, baz: null, abc: string|null, qwe: string|null, xyz: string|null}|array{foo: string, bar: null, baz: string, abc: string|null, qwe: string|null, xyz: string|null}|array{foo: string, bar: string, baz: null, abc: string|null, qwe: string|null, xyz: string|null}|array{foo: string, bar: string, baz: string, abc: string|null, qwe: string|null, xyz: string|null}
-23: Dumped type: non-empty-array<literal-string&non-falsy-string, string|null>&hasOffsetValue('abc', string|null)&hasOffsetValue('bar', string|null)&hasOffsetValue('baz', string|null)&hasOffsetValue('foo', string|null)&hasOffsetValue('qwe', string|null)&hasOffsetValue('xyz', string|null)
+19: Dumped type: array{foo: string|null, bar: string|null, baz: string|null, abc: string|null, qwe: string|null, xyz: string|null}
+23: Dumped type: array{foo: string|null, bar: string|null, baz: string|null, abc: string|null, qwe: string|null, xyz: string|null}
Full report
Line Error
19 `Dumped type: array{foo: string
23 `Dumped type: array{foo: string

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Sep 19, 2022

The original issue is not solved, but the others two examples are solved by phpstan/phpstan-src@43d3652...bd57fc5

Do you want to add a regression test @ondrejmirtes @rvanvelzen ? (I can do it if the test if wanted)

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 22, 2022

Fixed, thanks: phpstan/phpstan-src@5f1a4a5

@github-actions
Copy link

github-actions bot commented Oct 24, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants