Skip to content

Fix empty regex and empty alternation parse #3507

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 11, 2025
Merged

Fix empty regex and empty alternation parse #3507

merged 8 commits into from
Feb 11, 2025

Conversation

@mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Sep 27, 2024 •
edited
Loading

@mvorisek mvorisek force-pushed the fix_empty_alternation branch 2 times, most recently from b473d7f to bcd469c Compare September 27, 2024 13:36
staabm
staabm reviewed Sep 27, 2024
View reviewed changes
@mvorisek mvorisek force-pushed the fix_empty_alternation branch from bcd469c to d79f280 Compare September 27, 2024 21:28
@mvorisek mvorisek marked this pull request as ready for review September 27, 2024 22:38
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@mvorisek mvorisek force-pushed the fix_empty_alternation branch 2 times, most recently from 9f87f5a to 4c61b1f Compare September 27, 2024 23:20
@staabm
Copy link
Contributor

staabm commented Sep 29, 2024 •
edited
Loading

Do empty alternations or empty capturing groups have real world relevance?

I don't think I have ever seen a regex using it..?

@mvorisek
Copy link
Contributor Author

mvorisek commented Sep 29, 2024 •
edited
Loading

They definitely do, ~@(|phpstan-)param~ is a small example.

ondrejmirtes
ondrejmirtes requested changes Oct 6, 2024
View reviewed changes
tests/PHPStan/Command/IgnoredRegexValidatorTest.php Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This is a blocker, I can't merge this. This is the main reason why IgnoredRegexValidator exists at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ondrejmirtes if I understand the test correctly it was rejecting ...||... regex but the regex is valid, | is an alternation regex token and thus || means alternation with empty string which is fixed by this PR, would you mind amending the test and pushing to by branch directly?

Copy link
Member

Choose a reason for hiding this comment

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

IgnoredRegexValidator makes sure that if people have this in their configuration:

parameters:
	ignoreErrors:
		- '~Result of || is always true.~'

It makes PHPStan fail with the following error:

Invalid entry in ignoreErrors:
Ignored error ~Result of || is always true.~ has an unescaped '||' which leads to ignoring all errors. Use '\|\|' instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but it MUST NOT fail, as it is valid regex :)

So please tell me how to make everyone happy.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a valid regex for ignoreErrors.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe in December, after I release 2.0. Don't expect it sooner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bugfix into 1.12.x, might I understand why so late?

Copy link
Member

Choose a reason for hiding this comment

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

Because I have different priorities and this is not a severe bug affecting a lot of users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ondrejmirtes is now a better time?

About the usecase, see https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/v3.65.0/src/DocBlock/TypeExpression.php#L62 for example.

(a|b|) is shorter syntax of ((?:a|b|)?), the first one is currently now parsed by PHPStan and this PR fixes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ondrejmirtes, please review, the feedback was addressed.

@mvorisek mvorisek force-pushed the fix_empty_alternation branch from 4c61b1f to 7230e5d Compare October 6, 2024 17:42
@mvorisek mvorisek requested a review from ondrejmirtes October 7, 2024 18:39
@mvorisek mvorisek force-pushed the fix_empty_alternation branch 2 times, most recently from 84a8ecf to fa1f464 Compare October 9, 2024 05:03
@mvorisek mvorisek requested a review from staabm October 10, 2024 17:02
@mvorisek mvorisek force-pushed the fix_empty_alternation branch 3 times, most recently from 3c9c813 to 94ebfb5 Compare October 13, 2024 11:52
@mvorisek mvorisek force-pushed the fix_empty_alternation branch from 94ebfb5 to aec975a Compare November 20, 2024 11:45
@mvorisek mvorisek force-pushed the fix_empty_alternation branch from aec975a to efc81bc Compare December 13, 2024 10:36
@mvorisek mvorisek marked this pull request as draft February 4, 2025 14:53
@mvorisek mvorisek force-pushed the fix_empty_alternation branch from efc81bc to f40aae6 Compare February 4, 2025 23:30
@mvorisek mvorisek marked this pull request as ready for review February 4, 2025 23:34
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit bd1a196 into phpstan:1.12.x Feb 11, 2025
452 of 453 checks passed
@ondrejmirtes
Copy link
Member

Looks fine, thank you.

@ondrejmirtes ondrejmirtes mentioned this pull request Feb 11, 2025
Closed
@mvorisek
Copy link
Contributor Author

Thank you ❤

@mvorisek mvorisek deleted the fix_empty_alternation branch February 11, 2025 20:38
@staabm staabm mentioned this pull request May 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

@staabm staabm Awaiting requested review from staabm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mvorisek @phpstan-bot @staabm @ondrejmirtes