-
Notifications
You must be signed in to change notification settings - Fork 548
support concat of constant-string-union with a literal string #937
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
support concat of constant-string-union with a literal string #937
Conversation
|
I really don't want this, not without some kind of limitation. With each "if", the number of constant strings in a union type doubles. I can easily imagine a method with 20 ifs like that which leads to 1048576 constant strings in a union. People in practice will have methods with hundred ifs like that. I'd merge this only if the number of constant strings would be capped to something like 32 at maximum. |
95234d1 to
16b6896
Compare
|
thx for taking the patch into consideration.
I have added a limit of at max 32 items,... thats plenty for my use case. I think the failling e2e tests are unrelated, but I can't tell for sure. |
|
Can you please evaluate whether the error in the E2E test is legit or not? The source code is here: https://github.com/nikic/PHP-Parser/blob/bb87e28e7d7b8d9a7fda231d37457c9210faf6ce/lib/PhpParser/Node/Scalar/String_.php#L95 |
f069da6 to
9b7307a
Compare
|
the composer integration test error seems legit // $replacedName is Link|BasePackage|string|int|array{packageName: string, constraint: ConstraintInterface}|array{package: BasePackage}
$reason = 'They '.(count($literals) == 2 ? 'both' : 'all').' replace '.$replacedName.' and thus cannot coexist.';as phpstan now detects a error which it could not before in IMO this PR is ready to merge. |
src/Analyser/MutatingScope.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are the new lines. everything else was moved 1:1 from resolveType into this new method.
I figured it would make things a bit better to read in the future.. hopefully you are fine with it.
9b7307a to
1a79a20
Compare
1a79a20 to
a90a69f
Compare
|
Thank you. |
|
Thank you 🙏 |
we discussed this feature in phpstan/phpstan#6439 and even though ondrey mentioned some problems this could have, I figured implementing it would give us a better picture about possible downsides.
at least for me the implementation was easer then initally expected.
feel free to close this PR.
closes phpstan/phpstan#6439