- 
  Notifications
 You must be signed in to change notification settings 
- Fork 544
Add stringable-object pseudo type #1905
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
Conversation
Do you see how we could get rid of StringAlwaysAcceptingObjectWithToStringType and these hacks?
phpstan-src/src/Reflection/SignatureMap/NativeFunctionReflectionProvider.php
Lines 109 to 142 in 7e9ab72
Do you see how we could get rid of StringAlwaysAcceptingObjectWithToStringType and these hacks?
Yep, working on it :)
b18b4ba to
 db61fb5  
 Compare
 
 There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails (at least locally) because PHPStan is using the Symfony polyfill's definition of Stringable instead of the built-in :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't great, additionally because of the polyfill PHPStan isn't going to tell us about a nonexistent Stringable pre-PHP 8.0: https://phpstan.org/r/9e44f422-93e6-4923-9911-fe6f75940403
The solution is in this class: https://github.com/phpstan/phpstan-src/blob/1.9.x/src/Reflection/BetterReflection/BetterReflectionSourceLocatorFactory.php
We'd have to debug the ways Stringable is discovered before PHP 8 and ideally write a new source locator that would wrap an existing one so that it's not discovered by mistake in a polyfill. It's going to be different in phpstan-src (where the polyfill is known through composer.json) and it's going to be different when PHPStan is installed and used as PHAR (where the polyfill is probably known through AutoloadSourceLocator but also maybe AutoloadFunctionsSourceLocator).
Please note that there's already PhpVersionBlacklistSourceLocator that does this job in some cases.
But at the same time, we can't break projects that use the same polyfill and thus can rely on Stringable already existing before PHP 8.
The previous sentence also means we can't break analysis of phpstan-src itself 🤦♂️
So what I'd like to see:
- We need some integration tests around this. Which means to add them here https://github.com/phpstan/phpstan/blob/1.9.x/.github/workflows/other-tests.yml.
 a) Test that runs on PHP 7.4 and references Stringable - expect an error (run with a baseline file and the result should be green - means the error is present)
 b) Test that runs on PHP 8.0 and references Stringable - should be green without a baseline
 c) Test that runs on PHP 7.4, references Stringable, and has polyfills - should be green without a baseline
 d) Test that runs on PHP 8.0, references Stringable, and has polyfills - should be green without a baseline
- Add a new source locator wrapper for AutoloadSourceLocator so that polyfills from PHPStan's PHAR itself are skipped. Another namespace to skip would be Hoawhich is unprefixed in the PHAR because it's an old and messy library.
- Now we have green tests from 1) but we haven't solved our problem in phpstan-src 🤦♂️ But that solution doesn't have to be that clean. We can probably come up with some toggle that's used in https://github.com/phpstan/phpstan-src/blob/1.9.x/src/Testing/TestCase.neon and that makes polyfills being skipped in BetterReflectionSourceLocatorFactory even in phpstan-src.
db61fb5 to
 649313f  
 Compare
 
 There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it better to write
And add the rule on every level ?
This way if someone is level x but use
reportMaybe: true
it will be reported.
And on the opposite side, using
reportMaybe: false
disable the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule is on level 7. reportMaybes will never be false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule is on level 7. reportMaybes will never be false.
You can override the value in the config.
I already have/had project with config
parameters:
 level: 6
 reportMaybe: true
or
parameters:
 level: 7
 reportMaybe: false
This is useful when bumping level one by one.
It's a smaller step than bumping from 6 to 7 which is doing
parameters:
	checkUnionTypes: true
	reportMaybes: true
	checkListType: true
I can do one PR 6 + checkList, then later another PR with 6 + checklist and reportMaybe then a last PR with. 6 + everything, and then change the config to the level 7.
I feel like it's easier to opt-in, opt-out the rules when they are config dependent rather than level dependent. But that's a small difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're relying on internal config parameters that aren't documented on phpstan.org. This isn't a supported code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're relying on internal config parameters that aren't documented on phpstan.org. This isn't a supported code path.
Oh, I never thought there was internal config parameter. I see, I understand then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then, the following example https://phpstan.org/r/658bbf1b-b6d6-4b8e-a844-449c42cb75ed 
should be reported as an error on a lower level than the level 7
On PHP level 8 the check
$objectType->hasMethod('__toString')->no()
should be true. (I don't know if it does currently)
So it would make sens to have this rule on a lower level with the following check
if (
 $objectType->hasMethod('__toString')->no()
 || $this->reportMaybe && $objectType->hasMethod('__toString')->maybe()
) {
no ?
a5d90c1 to
 a69c7f0  
 Compare
 
 
This implements the
stringable-objecttype that Psalm also supports. A notable difference is that on PHP 8 it is simply an alias forStringable.On top of this type a new rule is also implemented that guard against possibly-invalid object-to-string casts. This only really applies to the simple
objecttype, for which the stringable-ness was not checked yet.