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

TypeSpecifier - understand \Composer\Pcre\Preg::match() == 1 same way as === 1 #3178

Closed
wants to merge 3 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jun 22, 2024

required for composer/pcre type narrowing analog

required for composer/pcre#24 (comment) to cover the ==1 and ===1 testcases

@@ -33,6 +33,7 @@
)
->ignoreErrorsOnPackage('phpunit/phpunit', [ErrorType::DEV_DEPENDENCY_IN_PROD]) // prepared test tooling
->ignoreErrorsOnPackage('jetbrains/phpstorm-stubs', [ErrorType::PROD_DEPENDENCY_ONLY_IN_DEV]) // there is no direct usage, but we need newer version then required by ondrejmirtes/BetterReflection
->ignoreErrorsOnPackage('composer/pcre', [ErrorType::SHADOW_DEPENDENCY]) // reverse dependency on composer/pcre to support https://github.com/composer/pcre/pull/24
Copy link
Contributor Author

@staabm staabm Jun 22, 2024

Choose a reason for hiding this comment

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

@janedbal is this the correct way to handle a optional runtime dependency?

@staabm
Copy link
Contributor Author

staabm commented Jun 22, 2024

Alternativly to this PR we could ask composer/pcre to turn the 0|1 returning api into a real bool (bc break).

And a third alternative could be that we handle callables which return 0|1 the same as bool

@ondrejmirtes
Copy link
Member

This isn't right, I don't want to hardcode things about other people's code in PHPStan's core.

The right way to handle this is to let TypeSpecifierContext know about various comparison operators and values on the other side of the comparison. So that extensions can react to this information.

@staabm
Copy link
Contributor Author

staabm commented Jun 22, 2024

So you say we need a new extension type?

@ondrejmirtes
Copy link
Member

Not new extension type, we'd need to find a way how to pass this information in TypeSpecifierContext.

@staabm
Copy link
Contributor Author

staabm commented Jun 23, 2024

ok, I need to think about that

@staabm staabm closed this Jun 23, 2024
@staabm staabm deleted the composer-preg branch June 23, 2024 07:56
@ondrejmirtes
Copy link
Member

My idea: right now we have methods null(), true(), truthy(), falsy(), false().

The new context called for extensions in these cases would return false for all of the current methods.

But there could be a new method called comparison() and it would return information the extension could act on.

@staabm
Copy link
Contributor Author

staabm commented Jun 25, 2024

I will give it a try right now

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.

2 participants