Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 4, 2024

by detecting static string literals in the regex pattern, we make constant-array result-types of the patterns more different.
this helps the TypeCombinator to produce tagged unions more often, which can result in more precise overall types.

first step into the direction described in phpstan/phpstan#11443 (comment)

@staabm staabm marked this pull request as ready for review August 4, 2024 07:05
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@staabm
Copy link
Contributor Author

staabm commented Aug 4, 2024

phpstan-nette needs some expectation adjustments after merge. I will work on it after this one landed.

@@ -111,28 +113,39 @@ private function walkRegexAst(
array &$groupCombinations,
array &$markVerbs,
bool $captureOnlyNamed,
bool $repeatedMoreThenOnce,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Than

@ondrejmirtes ondrejmirtes merged commit c6c06b1 into phpstan:1.11.x Aug 4, 2024
348 of 349 checks passed
@ondrejmirtes
Copy link
Member

This is becoming ridiculously good! Thank you.

@staabm staabm deleted the literals branch August 4, 2024 07:34
@Seldaek
Copy link
Contributor

Seldaek commented Aug 4, 2024

Woah this is awesome! Makes me kinda want to handle foo? as 'fo'|'foo' for example.. And even character classes with very few possible outcomes could be done that way like [abc] can just be one of those 3.. Not sure if it'd bring enough value but as a regex nerd it just sounds cool. This may need to be forked as a regex static analysis lib tho 😅 I'm only half kidding this is getting so advanced it feels sad that it's not reusable by others.

@staabm
Copy link
Contributor Author

staabm commented Aug 5, 2024

@Seldaek character classes have already been implemented with #3285

this is getting so advanced it feels sad that it's not reusable by others.

The current impl is tightly coupled with the PHPStan type-system. Its not that easy to be reused.

I think we need a concrete use-case to see whether re-using can make sense.

@Seldaek
Copy link
Contributor

Seldaek commented Aug 5, 2024

Damn you've been on fire these last few days. I'm on vacation so haven't kept track too closely.

@@ -21,6 +21,7 @@
use PHPStan\Type\TypeCombinator;
use function array_key_exists;
use function count;
use function implode;
Copy link

@mvorisek mvorisek Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is some issue - https://phpstan.org/r/78e9a062-ad8b-4a3d-a944-e671a4f17a9d - index 1 is optional, ie. the type is missing |array{string}

wider repro - https://phpstan.org/r/89288a2e-96e9-4c48-8419-3adc807bea74 - the last type is wrong too, the 1 and 2 indexes are definitely not present when index 3 is

Copy link
Contributor Author

@staabm staabm Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Index 1/2 is present but empty when 3 exists: https://3v4l.org/VgCUJ

I agree there is a constant type problem though

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the last type is wrong too, the 1 and 2 indexes are definitely not present when index 3 is

sorry, I meant empty string of course

the 2nd repro would be best fixed by "conditional types", ie. like if the inner capturing group type were dependent on the outer capturing group type

Copy link
Contributor Author

@staabm staabm Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please open issues with reproducing snippets for the known problems. there are too many open todos in already-merged PRs and I can't keep track of all of them

thank you

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.

5 participants