added AccessoryNonFalsyStringType by staabm · Pull Request #1542 · phpstan/phpstan-src

staabm · 2022-07-24T15:07:57Z

implements non-falsy-string

closes phpstan/phpstan#5317
closes phpstan/phpstan#5370

src/Type/Accessory/AccessoryNonEmptyStringType.php

ondrejmirtes

This isn't enough. You need to add PHPStan\Type\Type::isNonFalsyString().

src/Type/Accessory/AccessoryNonFalsyStringType.php

ondrejmirtes

Go through all the places where new AccessoryNonEmptyStringType() is created and think about whether AccessoryNonFalsyStringType also belongs there. An example - ConstantStringType::generalize().

herndlm · 2022-07-24T15:46:48Z

Does this also need an addition to the typespecifier? Smth that adds the type if the string is non-empty and not '0'?

ondrejmirtes · 2022-07-24T15:48:43Z

Add a type inference test and you'll see :) My tip is that we'll have to modify StringType::tryRemove(). Which falls under the "Go through all the places where new AccessoryNonEmptyStringType() is created and think about whether AccessoryNonFalsyStringType also belongs there." todo item :)

staabm · 2022-07-24T16:17:03Z

src/Type/Accessory/AccessoryNonFalsyStringType.php

memo to me: think about this case

ondrejmirtes · 2022-07-24T19:17:03Z

src/Type/IntersectionType.php

This doesn't need hacks in IntersectionType.

AccessoryNonEmptyStringType = AccessoryNonFalsyStringType + '0'

AccessoryNonFalsyStringType = AccessoryNonEmptyStringType - '0'

This means that AccessoryNonFalsyStringType is "smaller".

AccessoryNonFalsyStringType->isSuperTypeOf(AccessoryNonEmptyStringType) = maybe (makes sense, not every AccessoryNonEmptyStringType is AccessoryNonFalsyStringType)

AccessoryNonEmptyStringType->isSuperTypeOf(AccessoryNonFalsyStringType) = yes (makes sense, all AccessoryNonFalsyStringType are AccessoryNonEmptyStringType)

isSuperTypeOf implemented in 45c5c6a.

the hacks we see here are required to make sure a intersection cannot contain non-falsy-string and non-empty-string at the same time.

I guess this need to be handled via isSubTypeOf or tryRemove or similar instead, to get rid of these IntersectionType changes

I think its finally covered with 30728d4 and 7fd0884

I cannot get rid of this hack in IntersectionType atm, without regressing tests.

ondrejmirtes · 2022-07-24T19:47:12Z

Hi, I'm unsubscribing from this PR. At this rate, I expect at least 100 pushes from you (and I'm getting each one as a separate email). Of course, you could choose to develop it locally and push it once you consider it done from your side, so I can review it.

Please ping me via another channel once you consider it done and I can take a look. Thanks for understanding!

staabm · 2022-07-24T20:22:53Z

I have no idea yet, where the current error is coming from (and whether its legit or not)

1) PHPStan\Analyser\LegacyNodeScopeResolverTest::testBinaryOperations with data set #291 ('string|false', '$simpleXMLReturningXML')
$simpleXMLReturningXML at die
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'string|false'
+'''|'0'|non-falsy-string|false'
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:8549
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:8456
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:8459
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:2998

Edit: fixed with 7fd0884

staabm · 2022-07-25T07:24:30Z

Hi, I'm unsubscribing from this PR. At this rate, I expect at least 100 pushes from you (and I'm getting each one as a separate email). Of course, you could choose to develop it locally and push it once you consider it done from your side, so I can review it.

yeah thats fine. I don't expect reviews of PRs as long as they are in draft state.

pushing the intermediate state into a PR makes it easier for me to sync the work across different computers.

staabm · 2022-07-25T07:50:06Z

src/Type/StringType.php

a few lines below there is a if ($typeToRemove instanceof AccessoryNonEmptyStringType) { case.

I guess we also need a if ($typeToRemove instanceof AccessoryNonFalsyStringType) { case there, but wasn't able yet to come up with a test-case

staabm · 2022-07-25T08:18:35Z

Notes to myself:

handling non-falsy-string within StrContainingTypeSpecifyingExtension should be a followup IMO.

do we need non-falsy-string handling in TypeCombinator?

phpstan-src/src/Type/TypeCombinator.php

Lines 402 to 416 in 72f4b58

    
           if ( 
        
           	$a instanceof ConstantStringType 
        
           	&& $a->getValue() === '' 
        
           	&& $b->describe(VerbosityLevel::value()) === 'non-empty-string' 
        
           ) { 
        
           	return [null, new StringType()]; 
        
           } 
        
           if ( 
        
           	$b instanceof ConstantStringType 
        
           	&& $b->getValue() === '' 
        
           	&& $a->describe(VerbosityLevel::value()) === 'non-empty-string' 
        
           ) { 
        
           	return [new StringType(), null]; 
        
           }

herndlm · 2022-07-25T08:29:25Z

I have no idea yet, where the current error is coming from (and whether its legit or not)

1) PHPStan\Analyser\LegacyNodeScopeResolverTest::testBinaryOperations with data set #291 ('string|false', '$simpleXMLReturningXML')
$simpleXMLReturningXML at die
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'string|false'
+'''|'0'|non-falsy-string|false'
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:8549
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:8456
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:8459
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:2998

yeah that one is weird, a non-falsy-string that can also be an empty string or the string '0' is actually just string, right?

tests/PHPStan/Analyser/data/bitwise-not.php

staabm · 2022-07-25T09:12:58Z

Go through all the places where new AccessoryNonEmptyStringType() is created and think about whether AccessoryNonFalsyStringType also belongs there. An example - ConstantStringType::generalize().

done

Does this also need an addition to the typespecifier? Smth that adds the type if the string is non-empty and not '0'?

done

staabm · 2022-07-25T10:59:00Z

I think this one is now good to review.

I have 2 failling tests, I am not sure what todo about/how to fix.
all other issues I am aware of are commented in the PR.

src/Type/Accessory/AccessoryNumericStringType.php

ondrejmirtes

Also I'd like some "accepts" tests, for example in CallMethodsRuleTest.php

Let's have a method that accepts non-falsy-string. Are these being accepted, or reported?

'0'
'non-falsy-string'
'non-empty-string'
'string'

ondrejmirtes · 2022-07-26T16:23:15Z

I pushed some additional tests and a fix. There's still one test failing, the expected result makes more sense to me. The problem is still gonna be in AccessoryNonEmptyStringType::isSuperTypeOf() + isSubTypeOf() and the same methods in AccessoryNonFalsyStringType.

staabm · 2022-07-27T19:45:46Z

I don't like the final fix, but at least we are all green now :-)

staabm · 2022-07-27T20:03:22Z

src/Type/Accessory/AccessoryNonFalsyStringType.php

Need a explicit check for non-empty-string, instead of isNonEmptyString(), which would also be true for numeric-string, which we don‘t want here

you could still avoid it via the methods on the Type interface, right? Since there is also a isNumericString I mean.

I tried expressing it differently, but it would need to take care of at least class-string, non-falsy-string, numeric-string, constant-string.
it feels the instanceof check is better suited here

staabm · 2022-07-28T18:15:17Z

src/PhpDoc/TypeNodeResolver.php

just noting here so it will not be forgotten:
on twitter there was a discussion going on whether this type should better be named truthy-string instead (and keep 'non-falsy-string' as a deprecated alias)

closes phpstan/phpstan#5317

ondrejmirtes · 2022-08-05T20:46:34Z

Thank you!

herndlm reviewed Jul 24, 2022

View reviewed changes

src/Type/Accessory/AccessoryNonEmptyStringType.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Jul 24, 2022

View reviewed changes

src/Type/Accessory/AccessoryNonFalsyStringType.php Outdated Show resolved Hide resolved

src/Type/Accessory/AccessoryNonFalsyStringType.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Jul 24, 2022

View reviewed changes

staabm commented Jul 24, 2022

View reviewed changes

ondrejmirtes requested changes Jul 24, 2022

View reviewed changes

staabm force-pushed the non-falsy branch from 7fe0c58 to 23a19be Compare July 24, 2022 20:16

staabm commented Jul 25, 2022

View reviewed changes

rvanvelzen reviewed Jul 25, 2022

View reviewed changes

tests/PHPStan/Analyser/data/bitwise-not.php Outdated Show resolved Hide resolved

staabm force-pushed the non-falsy branch from 7e131fa to 7fd0884 Compare July 25, 2022 10:53

staabm marked this pull request as ready for review July 25, 2022 10:56

ondrejmirtes requested changes Jul 25, 2022

View reviewed changes

src/Type/Accessory/AccessoryNumericStringType.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Jul 25, 2022

View reviewed changes

staabm mentioned this pull request Jul 26, 2022

concat of 2 non-empty-strings should lead to non-falsy-string vimeo/psalm#8315

Closed

clxmstaab force-pushed the non-falsy branch from e53d146 to bd1ea41 Compare July 26, 2022 07:35

ondrejmirtes force-pushed the non-falsy branch from 98fd975 to 889d675 Compare July 26, 2022 15:44

staabm force-pushed the non-falsy branch from bab8e37 to 8e7266b Compare July 27, 2022 19:02

staabm commented Jul 27, 2022

View reviewed changes

staabm commented Jul 28, 2022

View reviewed changes

staabm and others added 7 commits August 5, 2022 22:45

added AccessoryNonFalsyStringType

19464a3

closes phpstan/phpstan#5317

added regression test for phpstan/phpstan#5370

4845431

use traits

33784ec

added PHPStan\Type\Type::isNonFalsyString()

b73b3a8

TypeCombinator tests

1cc544d

Fix bug

83c22ba

fix

d4110a6

ondrejmirtes force-pushed the non-falsy branch from 9f067dc to d4110a6 Compare August 5, 2022 20:46

ondrejmirtes merged commit e0f84e6 into phpstan:1.8.x Aug 5, 2022

staabm deleted the non-falsy branch August 6, 2022 06:08

staabm mentioned this pull request Aug 12, 2024

Narrowing to non-falsy-string doesn't work if '' and '0' equality checks are swapped phpstan/phpstan#9114

Open

Conversation

staabm commented Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

herndlm commented Jul 24, 2022

Uh oh!

ondrejmirtes commented Jul 24, 2022

Uh oh!

staabm Jul 24, 2022

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

staabm Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

staabm Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Jul 24, 2022

Uh oh!

staabm commented Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm commented Jul 25, 2022

Uh oh!

staabm Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

staabm commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

herndlm commented Jul 25, 2022

Uh oh!

Uh oh!

staabm commented Jul 25, 2022

Uh oh!

staabm commented Jul 25, 2022

Uh oh!

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Jul 26, 2022

Uh oh!

staabm commented Jul 27, 2022

Uh oh!

staabm Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

herndlm Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

staabm Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Aug 5, 2022

Uh oh!

Reviewers

Assignees

Labels

staabm commented Jul 24, 2022 •

edited

Loading

ondrejmirtes Jul 24, 2022 •

edited

Loading

staabm commented Jul 24, 2022 •

edited

Loading

staabm commented Jul 25, 2022 •

edited

Loading

staabm Jul 27, 2022 •

edited

Loading

staabm Jul 28, 2022 •

edited

Loading