Skip to content

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Jan 22, 2022

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

@staabm staabm marked this pull request as draft January 22, 2022 11:49
@staabm staabm marked this pull request as ready for review January 22, 2022 21:00
@ondrejmirtes
Copy link
Member

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.

@clxmstaab clxmstaab force-pushed the union-string-literal-concat branch from 95234d1 to 16b6896 Compare January 24, 2022 12:30
@staabm
Copy link
Contributor Author

staabm commented Jan 24, 2022

thx for taking the patch into consideration.

I'd merge this only if the number of constant strings would be capped to something like 32 at maximum.

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.

@ondrejmirtes
Copy link
Member

Can you please evaluate whether the error in the E2E test is legit or not?

{"totals":{"errors":0,"file_errors":6},"files":{"/home/runner/work/phpstan-src/phpstan-src/tests/e2e/PHP-Parser/lib/PhpParser/Node/Scalar/String_.php":{"errors":6,"messages":[{"message":"Regex pattern is invalid: Empty regular expression in pattern: ","line":95,"ignorable":true},{"message":"Regex pattern is invalid: No ending delimiter ')' found in pattern: )~","line":95,"ignorable":true},{"message":"Regex pattern is invalid: No ending delimiter '|' found in pattern: |u\\{([0-9a-fA-F]+)\\}","line":95,"ignorable":true},{"message":"Regex pattern is invalid: No ending delimiter '|' found in pattern: |u\\{([0-9a-fA-F]+)\\})~","line":95,"ignorable":true},{"message":"Regex pattern is invalid: No ending delimiter '~' found in pattern: ~\\\\([\\\\$nrtfve]|[xX][0-9a-fA-F]{1,2}|[0-7]{1,3}","line":95,"ignorable":true},{"message":"Regex pattern is invalid: No ending delimiter '~' found in pattern: ~\\\\([\\\\$nrtfve]|[xX][0-9a-fA-F]{1,2}|[0-7]{1,3}|u\\{([0-9a-fA-F]+)\\}","line":95,"ignorable":true}]}},"errors":[]}

The source code is here: https://github.com/nikic/PHP-Parser/blob/bb87e28e7d7b8d9a7fda231d37457c9210faf6ce/lib/PhpParser/Node/Scalar/String_.php#L95

@staabm staabm marked this pull request as draft January 25, 2022 19:41
@clxmstaab clxmstaab force-pushed the union-string-literal-concat branch from f069da6 to 9b7307a Compare March 11, 2022 15:27
@staabm
Copy link
Contributor Author

staabm commented Mar 11, 2022

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.';
 ------ ----------------------------------------------------------------------- 
  Line   src/Composer/DependencyResolver/Rule.php                               
 ------ ----------------------------------------------------------------------- 
  383    Binary operation "." between 'They all replace '|'They both replace '  
         and array{package: Composer\Package\BasePackage}|array{packageName:    
         string, constraint:                                                    
         Composer\Semver\Constraint\ConstraintInterface}|Composer\Package\Base  
         Package|Composer\Package\Link|int|string results in an error.          
 ------ -----------------------------------------------------------------------

as phpstan now detects a error which it could not before in

https://github.com/composer/composer/blob/3ae111140facdba8ae82adcd1085e4adfc7d715c/src/Composer/DependencyResolver/Rule.php#L383

IMO this PR is ready to merge.

@staabm staabm marked this pull request as ready for review March 11, 2022 15:44
Comment on lines +2671 to +2701
Copy link
Contributor Author

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.

@staabm staabm force-pushed the union-string-literal-concat branch from 9b7307a to 1a79a20 Compare March 14, 2022 18:11
@ondrejmirtes ondrejmirtes force-pushed the union-string-literal-concat branch from 1a79a20 to a90a69f Compare March 17, 2022 13:01
@ondrejmirtes ondrejmirtes merged commit fbba892 into phpstan:1.5.x Mar 17, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@staabm
Copy link
Contributor Author

staabm commented Mar 17, 2022

Thank you 🙏

@staabm staabm deleted the union-string-literal-concat branch April 4, 2022 09:42
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.

union of literal strings in types after if conditions

3 participants