Skip to content

Commit

Permalink
Precise type for $matches from preg_match generally available, ou…
Browse files Browse the repository at this point in the history 
…t of bleeding edge
  • Loading branch information
@ondrejmirtes
ondrejmirtes committed Aug 1, 2024
1 parent c541ce1 commit bd2cec1
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 7 deletions.
1 change: 0 additions & 1 deletion conf/bleedingEdge.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ parameters:
paramOutType: true
pure: true
checkParameterCastableToStringFunctions: true
narrowPregMatches: true
uselessReturnValue: true
printfArrayParameters: true
preciseMissingReturn: true
Expand Down
15 changes: 10 additions & 5 deletions conf/config.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ parameters:
paramOutType: false
pure: false
checkParameterCastableToStringFunctions: false
narrowPregMatches: false
uselessReturnValue: false
printfArrayParameters: false
preciseMissingReturn: false
Expand Down Expand Up @@ -290,10 +289,6 @@ conditionalTags:
phpstan.parser.richParserNodeVisitor: %featureToggles.curlSetOptTypes%
PHPStan\Parser\TypeTraverserInstanceofVisitor:
phpstan.parser.richParserNodeVisitor: %featureToggles.instanceofType%
PHPStan\Type\Php\PregMatchTypeSpecifyingExtension:
phpstan.typeSpecifier.functionTypeSpecifyingExtension: %featureToggles.narrowPregMatches%
PHPStan\Type\Php\PregMatchParameterOutTypeExtension:
phpstan.functionParameterOutTypeExtension: %featureToggles.narrowPregMatches%

services:
-
Expand Down Expand Up @@ -1491,6 +1486,16 @@ services:
-
class: PHPStan\Type\Php\PregMatchTypeSpecifyingExtension

-
class: PHPStan\Type\Php\PregMatchTypeSpecifyingExtension
tags:
- phpstan.typeSpecifier.functionTypeSpecifyingExtension

-
class: PHPStan\Type\Php\PregMatchParameterOutTypeExtension
tags:
- phpstan.functionParameterOutTypeExtension

-
class: PHPStan\Type\Php\PregMatchParameterOutTypeExtension

Expand Down
1 change: 0 additions & 1 deletion conf/parametersSchema.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ parametersSchema:
paramOutType: bool()
pure: bool()
checkParameterCastableToStringFunctions: bool()
narrowPregMatches: bool()
uselessReturnValue: bool()
printfArrayParameters: bool()
preciseMissingReturn: bool()
Expand Down

7 comments on commit bd2cec1

@ondrejmirtes
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @staabm @Seldaek I'm going to enable this for everyone in PHPStan 1.12, it's not a BC break, it's just smarter analysis. The time in bleeding edge in 1.11 allowed us to test this without greater impact because of bugs, but I think it's solid now. I'd welcome the extension for preg_match_all and preg_replace_callback too. Thanks.

@staabm
Copy link
Contributor

@staabm staabm commented on bd2cec1 Aug 1, 2024

Choose a reason for hiding this comment

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

this might be a problem for composer/pcre which relies on the feature-flag. this might mean composer/pcre needs to define a phpstan-version conflict with 1.11.x when 1.12.x is released as it cannot be compatible with both versions (the parameter schema removal is a BC break, I guess)

@ondrejmirtes
Copy link
Member Author

Choose a reason for hiding this comment

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

I understand why you used it.

  1. Please send a PR that puts it back in parametersSchema (but setting it either to true or false doesn't do anything).
  2. It's not a BC break because the contents of featureToggles is not documented. Only options documented on https://phpstan.org/config-reference are part of BC promise.

But again, I'm making an exception for this one.

@thg2k
Copy link
Contributor

@thg2k thg2k commented on bd2cec1 Aug 1, 2024

Choose a reason for hiding this comment

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

@ondrejmirtes
Would it be possible to make the featureToggles part a generic array<string, mixed> at parameterSchema level?
I run a script that runs the analysis of my codebase on multiple phpstan versions to catch regressions/improvements. So far I played only with some feature flags that existed throughout 1.10.x and 1.11.x but I was exactly wondering how to solve the problem once it will occur.

Having the featureToggles a generic key-value map would solve the problem by generally ignoring what it doesn't know, instead of complaining about it.

@ondrejmirtes
Copy link
Member Author

Choose a reason for hiding this comment

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

@thg2k I guess you should always grab the current schema and filter the toggles you're trying through that. Or expect a validation error and fix it once it occurs.

@thg2k
Copy link
Contributor

@thg2k thg2k commented on bd2cec1 Aug 1, 2024

Choose a reason for hiding this comment

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

@thg2k I guess you should always grab the current schema and filter the toggles you're trying through that. Or expect a validation error and fix it once it occurs.

That's not what I meant, I mean that if version 2 introduces a featureToggle: X, and I enable it in my config file, it will not work in version 1 and version 3 if they don't have it. I'd like to run the same config file on all versions at the same time...

@staabm
Copy link
Contributor

@staabm staabm commented on bd2cec1 Aug 1, 2024

Choose a reason for hiding this comment

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

  1. Please send a PR that puts it back in parametersSchema (but setting it either to true or false doesn't do anything).

I discussed it with Jordi and we came to the conclusion that we - fearless developers as we are - will drop the feature-flag usage of composer/pcre, so no additional BC stuff is needed at PHPStan-src level ;-)

Please sign in to comment.