Skip to content

Fix memory error when trying to represent big literal string types #1088

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 20, 2022
Merged

Fix memory error when trying to represent big literal string types #1088

merged 1 commit into from
Mar 20, 2022

Conversation

mathroc
Copy link
Contributor

@mathroc mathroc commented Mar 19, 2022

return a non-empty-string type instead when length is > 100

fix phpstan/phpstan#6866

@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch 4 times, most recently from 017bbc9 to 94261e2 Compare March 19, 2022 23:31
@mathroc mathroc marked this pull request as ready for review March 19, 2022 23:35
ondrejmirtes
ondrejmirtes requested changes Mar 20, 2022
View reviewed changes
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.

  1. Please write a separate test in AnalyserIntegrationTest (see the other test cases there). Those are mostly for memory crashes and performance hogs like you are trying to fix.
  2. Please add another case that's exactly under the precision limit.

Otherwise this fix is great and exactly how I'd do it 😊

@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch from 94261e2 to 8c3226b Compare March 20, 2022 12:20
staabm
staabm reviewed Mar 20, 2022
View reviewed changes
tests/PHPStan/Analyser/data/bug-6866.php Outdated
@@ -0,0 +1,8 @@
<?php

namespace Bug6740;
Copy link
Contributor

Choose a reason for hiding this comment

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

Namespace has a different bug number then the filename.

And another bug number in the
Method name

tests/PHPStan/Analyser/AnalyserIntegrationTest.php Outdated
@@ -569,6 +570,16 @@ public function testBug6740(): void
$this->assertNoErrors($errors);
}

public function testBug1088(): void
{
$str = str_repeat('a', 99);
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 lines should be moved into the data/*bug added file.

assertType is meant for use in these data/* files

@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch from 8c3226b to 79acc55 Compare March 20, 2022 12:30
staabm
staabm reviewed Mar 20, 2022
View reviewed changes
tests/PHPStan/Analyser/data/bug-6866.php Outdated

use function PHPStan\Testing\assertType;

$str = str_repeat('a', 99);
Copy link
Contributor

@staabm staabm Mar 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Code in data/* files should be within functions or class-methods, so it won't be executed while running tests, but just been analyzed by phpstan.

Method/Class/Function-names are not relevant

Otherwise looks great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ah this is real-time feedback :)

thank you for your help!

@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch 2 times, most recently from 6c6ce29 to c8353db Compare March 20, 2022 12:38
staabm
staabm reviewed Mar 20, 2022
View reviewed changes
tests/PHPStan/Analyser/data/bug-6866.php Outdated
function f(): void
{
$str = str_repeat('a', 99);
assertType('"' . $str . '"', $str);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I mixed it up. The analyserIntegaration data/* files do not require the assertType calls.

You put the code in there which should work as is - see other data/ files beeing analysed by the this TestCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok. So, should I move the assertType(...) part of the test back to data/literal-string.php and leave only the standalone str_repeat() in here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think so

@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch 4 times, most recently from 7cb0b4c to 8520200 Compare March 20, 2022 13:34
ondrejmirtes
ondrejmirtes requested changes Mar 20, 2022
View reviewed changes
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.

Remove the underscores in the number, they don't work in all PHP versions that are tested here. Otherwise looks fine 😊

@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch from 8520200 to 1994c81 Compare March 20, 2022 13:40
ondrejmirtes
ondrejmirtes requested changes Mar 20, 2022
View reviewed changes
tests/PHPStan/Analyser/data/literal-string.php Outdated
@@ -27,6 +27,9 @@ public function doFoo($literalString, string $string)
assertType('literal-string', str_repeat($literalString, 10));
assertType("''", str_repeat('', 10));
assertType("'foofoofoofoofoofoofoofoofoofoo'", str_repeat('foo', 10));
$str = str_repeat('a', 99);
assertType("'$str'", $str);
Copy link
Member

Choose a reason for hiding this comment

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

Alright, one more thing, sorry. This isn't a useful test, it's a tautology and we don't know what's getting tested here. Just repeat 'a' 99 times in the string, thanks.

@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch from 1994c81 to 1179066 Compare March 20, 2022 13:55
@mathroc
Fix memory error when trying to represent big literal string types
8888d43 
return a non-empty-string type instead when length is > 100
@mathroc mathroc force-pushed the fix/memory-error-with-big-literal-string-type branch from 1179066 to 8888d43 Compare March 20, 2022 13:55
@mathroc
Copy link
Contributor Author

mathroc commented Mar 20, 2022

ok, so everything is green except one "Compile PHAR" check, but I can't see what's going on, GitHub return a 500 "unicorn" page when trying to see the details 🤷

let me know if I can do anything about this

@ondrejmirtes ondrejmirtes merged commit 59de1f8 into phpstan:1.5.x Mar 20, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@mathroc mathroc deleted the fix/memory-error-with-big-literal-string-type branch March 20, 2022 15:18
@staabm
Copy link
Contributor

staabm commented Mar 20, 2022

Well done @mathroc

@herndlm
Copy link
Contributor

herndlm commented Mar 21, 2022 •
edited
Loading

Hey, sorry, I don't want to be annoying, but shouldn't this have been merged/cherry-picked to 1.4.x (too)?
UPDATE: except there will be no regular 1.4.x release anymoren, then it makes sense. Sorry for the noise :)

@ondrejmirtes
Copy link
Member

Yeah, I've kept 1.4.x around to issue hotfixes if something really bad happens in 1.4.9 :) But I don't plan to release anything in 1.4.x anymore.

BTW thanks to some changes I made, I'll be able to release 1.5.x more easily after 1.6.x is the default branch :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staabm staabm staabm left review comments

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

memory error in StrRepeatFunctionReturnTypeExtension.php
4 participants
@mathroc @ondrejmirtes @staabm @herndlm