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

Fix negative offset false positive on constant string #3784

Merged
merged 3 commits into from
Feb 12, 2025

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jan 16, 2025

@ondrejmirtes
Copy link
Member

The bug report is about an error, the regression test should be in RuleTestCase.

if ($offsetType instanceof ConstantIntegerType) {
if ($offsetType->getValue() < strlen($this->value)) {
if ($strLenType->isSuperTypeOf($offsetType)->yes()) {
return new self($this->value[$offsetType->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.

this change also fixes a php warning which could happen on too small negative offsets (covered by the tests)

@staabm staabm force-pushed the bug12122 branch 2 times, most recently from 89c7f69 to f5c2054 Compare January 24, 2025 13:20
@@ -366,7 +366,7 @@ public function isUppercaseString(): TrinaryLogic
public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
if ($offsetType->isInteger()->yes()) {
$strLenType = IntegerRangeType::fromInterval(0, strlen($this->value) - 1);
$strLenType = IntegerRangeType::fromInterval(-strlen($this->value), strlen($this->value) - 1);
Copy link
Member

Choose a reason for hiding this comment

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

You know you could ask for strlen() just once :)

@staabm staabm force-pushed the bug12122 branch 3 times, most recently from feae16d to 7afe44a Compare February 5, 2025 12:20
@ondrejmirtes ondrejmirtes merged commit 14faee1 into phpstan:1.12.x Feb 12, 2025
451 of 452 checks passed
@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.

2 participants