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

added AccessoryNonFalsyStringType #1542

Merged
merged 7 commits into from
Aug 5, 2022
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 24, 2022

implements non-falsy-string

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

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

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

@herndlm
Copy link
Contributor

herndlm commented Jul 24, 2022

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

@ondrejmirtes
Copy link
Member

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 :)

Comment on lines +111 to +117
if ((new ConstantIntegerType(0))->isSuperTypeOf($offsetType)->yes()) {
return new IntersectionType([new StringType(), new AccessoryNonFalsyStringType()]);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

memo to me: think about this case

foreach ($this->getSortedTypes() as $type) {
if ($type instanceof AccessoryNonEmptyStringType
|| $type instanceof AccessoryLiteralStringType
|| $type instanceof AccessoryNumericStringType
|| $type instanceof AccessoryNonFalsyStringType
) {
if ($type instanceof AccessoryNonFalsyStringType) {
Copy link
Member

@ondrejmirtes ondrejmirtes Jul 24, 2022

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its finally covered with 30728d4 and 7fd0884

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@ondrejmirtes
Copy link
Member

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
Copy link
Contributor Author

staabm commented Jul 24, 2022

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
Copy link
Contributor Author

staabm commented Jul 25, 2022

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.

public function isLiteralString(): TrinaryLogic
{
return TrinaryLogic::createMaybe();
}

public function tryRemove(Type $typeToRemove): ?Type
{
if ($typeToRemove instanceof ConstantStringType && $typeToRemove->getValue() === '0') {
return TypeCombinator::intersect($this, new AccessoryNonFalsyStringType());
}
if ($typeToRemove instanceof ConstantStringType && $typeToRemove->getValue() === '') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

staabm commented Jul 25, 2022

Notes to myself:

  • handling non-falsy-string within StrContainingTypeSpecifyingExtension should be a followup IMO.
  • do we need non-falsy-string handling in TypeCombinator?
    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
Copy link
Contributor

herndlm commented Jul 25, 2022

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?

@staabm
Copy link
Contributor Author

staabm commented Jul 25, 2022

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 staabm marked this pull request as ready for review July 25, 2022 10:56
@staabm
Copy link
Contributor Author

staabm commented Jul 25, 2022

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.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Contributor Author

staabm commented Jul 27, 2022

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

return $otherType->isSuperTypeOf($this);
}

if ($otherType instanceof AccessoryNonEmptyStringType) {
Copy link
Contributor Author

@staabm staabm Jul 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@staabm staabm Jul 28, 2022

Choose a reason for hiding this comment

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

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

@@ -242,6 +243,12 @@ private function resolveIdentifierTypeNode(IdentifierTypeNode $typeNode, NameSco
new AccessoryNonEmptyStringType(),
]);

case 'non-falsy-string':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

@ondrejmirtes
Copy link
Member

Thank you!

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.

FP: "If condition is always true." with simple string non-empty-string is not always truthy
4 participants