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

Keeps forgetting about @var doc comments #5921

Closed
dktapps opened this issue Nov 3, 2021 · 36 comments
Closed

Keeps forgetting about @var doc comments #5921

dktapps opened this issue Nov 3, 2021 · 36 comments

Comments

@dktapps
Copy link
Contributor

dktapps commented Nov 3, 2021

Bug report

I'm not sure exactly what triggers this. Hopefully someone reading this issue does.

I hate that this is one of those "it randomly happens" issues, but unfortunately this is the best I got right now.

Note that deleting PHPStan's cache in $TEMP/phpstan resolves the issue.

Summary

Analysing latest PM, PHPStan randomly forgets @var doc comments like these:

https://github.com/pmmp/PocketMine-MP/blob/c77829f4adee4d5fe61829823f4dbad545c100e9/src/entity/Human.php#L254
https://github.com/pmmp/PocketMine-MP/blob/b9b1ba95261f90e61657a0220ca8a59fa0dad9d7/src/entity/Living.php#L153
https://github.com/pmmp/PocketMine-MP/blob/3be9548b1ea240cdf352fa08a603cc3729f88682/src/network/mcpe/compression/CompressBatchTask.php#L48
https://github.com/pmmp/PocketMine-MP/blob/bb05af103d22148dbe3aa053a2119c2603282343/src/plugin/PluginManager.php#L497
https://github.com/pmmp/PocketMine-MP/blob/f827a555d5f98f22aea919f165853dff224b5176/src/utils/Utils.php#L178

which spontaneously leads to bogus errors like these:

 ------ -------------------------------------------------------------------------------------------------------------------------------------------------
  Line   src\entity\Human.php
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------
  256    Call to an undefined method pocketmine\nbt\tag\Tag::getByte().
  260    Parameter #1 $tag of static method pocketmine\item\Item::nbtDeserialize() expects pocketmine\nbt\tag\CompoundTag, pocketmine\nbt\tag\Tag given.
  262    Parameter #1 $tag of static method pocketmine\item\Item::nbtDeserialize() expects pocketmine\nbt\tag\CompoundTag, pocketmine\nbt\tag\Tag given.
  283    Call to an undefined method pocketmine\nbt\tag\Tag::getByte().
  283    Parameter #1 $tag of static method pocketmine\item\Item::nbtDeserialize() expects pocketmine\nbt\tag\CompoundTag, pocketmine\nbt\tag\Tag given.
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------

 ------ ----------------------------------------------------------------
  Line   src\entity\Living.php
 ------ ----------------------------------------------------------------
  157    Call to an undefined method pocketmine\nbt\tag\Tag::getByte().
  164    Call to an undefined method pocketmine\nbt\tag\Tag::getInt().
  165    Call to an undefined method pocketmine\nbt\tag\Tag::getByte().
  166    Call to an undefined method pocketmine\nbt\tag\Tag::getByte().
  167    Call to an undefined method pocketmine\nbt\tag\Tag::getByte().
 ------ ----------------------------------------------------------------

 ------ ----------------------------------------------------
  Line   src\network\mcpe\compression\CompressBatchTask.php
 ------ ----------------------------------------------------
  50     Cannot call method resolve() on mixed.
 ------ ----------------------------------------------------

 ------ -------------------------------------------------------------------------------------------------------------------------------
  Line   src\plugin\PluginManager.php
 ------ -------------------------------------------------------------------------------------------------------------------------------
  499    Parameter #1 $objectOrClass of class ReflectionClass constructor expects class-string<T of object>|T of object, string given.
 ------ -------------------------------------------------------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------------------------------------------------
  Line   src\utils\Utils.php
 ------ ---------------------------------------------------------------------------------------------------------------
  180    Method pocketmine\utils\Utils::cloneObjectArray() should return array<T of object> but returns array<object>.
 ------ ---------------------------------------------------------------------------------------------------------------

Deleting PHPStan's cache $TEMP/phpstan resolves the issue, but it keeps recurring.

Code snippet that reproduces the problem

Linked above, but like I said, it doesn't happen all the time.

Expected output

The above errors shouldn't be reported (they are bogus).

@mergeable
Copy link

mergeable bot commented Nov 3, 2021

This bug report is missing a link to reproduction on phpstan.org.

It will most likely be closed after manual review.

@dktapps dktapps changed the title Keeps forgetting about doc comments Keeps forgetting about @var doc comments Nov 3, 2021
@ondrejmirtes
Copy link
Member

Would be great if you could test it with phpstan-src and bisect it to a specific commit where it starts happening.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 3, 2021

This is gonna take ages then, because it doesn't happen all the time :/
It's been happening ever since I updated to 1.0. It never happened on 0.12.99 to the best of my memory.

@voku
Copy link
Contributor

voku commented Nov 4, 2021

I saw the same problem while I updated from 0.12.99 to 1.0.0, but after I fixed my extensions / config etc. it disappeared.

Can you show your current phpstan config?

@dktapps
Copy link
Contributor Author

dktapps commented Nov 4, 2021

@InvisibleSmiley
Copy link

I and some colleagues of mine also see this. Our projects are proprietary so I cannot share any detailed info but maybe the following helps:

  • adding a blank line in the vicinity of the reported issue fixes the issue (for the time being, i.e. until making other changes that trigger the issue again), and at least for me it doesn't reappear when removing the blank line again (but my colleague suggested this was not the case for him)
  • said other changes that trigger the issue could be anything that should make the cache partly invalid, like adding generics or changing config settings

One case where the issue appeared for me is like this:

<?php
class Foo {

public function getFields(): array { return []; }

/**
 * @param object $other
 * @return bool
 */
public function equals(object $other): bool {
    if (get_class($other) !== get_class($this)) {
        return false;
    }

    /** @var static $other */

    $myFields = $this->getFields();
    $otherFields = $other->getFields();

    return $myFields === $otherFields;
}

}

(I included the method PHPDoc in case that's relevant; I guess/hope it's not because it should be redundant.)

@staabm
Copy link
Contributor

staabm commented Nov 5, 2021

@InvisibleSmiley could you put that code into a repo in which the error can be reproduced?

@InvisibleSmiley
Copy link

I don't know how to reproduce it. All I'm saying is that the issue appeared in code similar to the example. No idea what's needed to trigger the issue. Hence no point in creating a repo for it at this stage.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 5, 2021

FWIW, the common theme always seems to be inline @var.

I'm waiting for the issue to recur so I can attempt to compare caches and see what caused it, but no luck so far.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 6, 2021

I just got a hit, but comparing caches seems like needle in a haystack:

$ git diff --shortstat --no-index ~/AppData/Local/Temp/phpstan-broken/ ~/AppData/Local/Temp/phpstan
 38979 files changed, 1414 insertions(+), 5200237 deletions(-)

I hope someone knows a better way to reproduce this, because I got nothing. Best I have is that it seems to recur when modifying a file that contains inline @var, but since it doesn't always happen I have no idea what the exact trigger is.

@knallcharge
Copy link

knallcharge commented Nov 8, 2021

I have the same problem, code that produces this is something like this:
https://phpstan.org/r/d5766240-d5bc-48a9-ab54-145cc89df82e
Of course the snippet does not produce the error, I'm getting "Cannot cast mixed to int" in line 12, even though $result is clearly marked as string|bool.

"Solution"

  • delete phpstan cache (Update: using clear-result-cache did not solve this for me, I had to set tmpDir to a completely different path) OR
  • rename the variable e.g. $result to $test, run phpstan, error disappears, rename variable to old name (error does not come back, until ...)

As I get hundreds of those errors, renaming variables can't be a solution, so I keep clearing the cache, but that also is not great as it rescans all files on the next run.
I can't really give any more hints as this only happens occasionally (started after updating to 1.0.0) and not on all files that contain a @var tag.

@knallcharge
Copy link

@dktapps
Do you also use PhpStorm and if so, do you see the reported errors there also?
Because I don't. I have no idea what might cause this, but only the console-output reports the (false) errors, when I switch to PhpStorm to the given file/line, there are no errors.
The only difference I see is that via console I call vendor\bin\phpstan while PhpStorm is configured to call vendor\bin\phpstan.bat. Don't know how this would make a difference as it seems to be a cache-problem, that came up after going from 0.12 to 1.0, just thought I leave a note in case it could matter somehow.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 9, 2021

PhpStorm analyses files copied to ~/AppData/Local/Temp/PHPStantemp_folder%d, so cache won't hit on such cases since the real file path is different. That's also the reason why file-specific ignoreErrors don't work in PhpStorm.

@knallcharge
Copy link

Ok, didn't know that, thanks for pointing it out, that explains the strange behavior :)

@knallcharge
Copy link

knallcharge commented Nov 11, 2021

I have done some more research on this, still can't reproduce it, but found something that may be related.
I'm also using rector, my setup is that phpstan.neon and rector.php are inside a build-directory and phpstan.neon is configured for rector like this: $parameters->set(Option::PHPSTAN_FOR_RECTOR_PATH, getcwd() . '/build/phpstan.neon'); (in case that may matter), using Windows.

What struck me was that I have two folders with cached phpstan-results:

grafik

  • running phpstan creates \AppData\Local\Temp\phpstan\cache\PHPStan
  • running rector creates \AppData\Local\Temp\cache\PHPStan

I created an error that gets reported in phpstan, ran phpstan and rector afterwards and was thinking that due to the second cache-directory maybe the error was transferred to the rector-cache which was now used by phpstan so the error seemingly would never disappear (as "clear-result-cache" only clears the directories created by phpstan but not the ones created by rector). Unfortunately, that did not happen, both phpstan and rector report no errors after fixing the one created for the test - which of course is the expected behavior. (Also tried the other way around: delete all caches, create error, run rector, remove error, run phpstan - same result: everything ok, no false positives.)
There's still a difference for now: \AppData\Local\Temp\phpstan\resultCache.php (which contains the error) exists, while \AppData\Local\Temp\cache\resultCache.php does not - don't know if under some weird circumstance this may change, if my findings are expected or if this whole issue is rather a problem with rector than with phpstan or even my setup (or a mixture of all).

As I said, I'm still not able to produce the error on purpose, I just thought I'd share my findings in case it puts somebody on the right track.

@herndlm
Copy link
Contributor

herndlm commented Nov 11, 2021

Interesting, could phpstan runs from other local projects influence the cache there somehow? I think phpstan would just re-create it then but would be interesting if this also occurs if a local cache is used, see #4237 (comment)

@knallcharge
Copy link

There are no other local projects that run phpstan, but: PhpStorm!

I usually run phpstan via the console to get a full list of warnings apart from the "live"-warnings I get in PhpStorm. But from time to time I run the code inspection in PhpStorm on the whole project and that seems to cause the problem. While I did a lot of testing (see my last post) and couldn't reproduce it, right after running PhpStorm's code inspection of my project, it came up with a new list of false positives that now show in phpstan even if run via the console!

Also strange: In PhpStorm's problem result window, I got 108 warnings from phpstan, but only a handful are actual warnings, all other are: "phpstan: PHPStan crashed in the previous run probably because of excessive memory consumption.
It consumed around 44 MB of memory. To avoid this issue, allow to use more memory with the --memory-limit option.
"
The thing is, that I have configured PhpStorm to grant 2G of memory to phpstan, so how come it complains about having used only 44MB?

@ondrejmirtes
Copy link
Member

If PhpStorm violates the takeaway from this article https://phpstan.org/blog/why-you-should-always-analyse-whole-project then sure, I know the reason why this is happening.

This is because of an optimization I did for PHPStan 1.0. Before AST is cached and if it's not in the analysed paths, the function bodies are removed because they're irrelevant for PHPStan. This makes sense for vendor directory.

What's probably happening in this case is that PHPStan just runs on a single file and the rest is considered 3rd party and therefore the @var is not seen by FileTypeMapper. Which then gets cached on disk and it's not refreshed when the file is analysed again.

I see two possible solutions:

  1. Let CleaningParser preserve all inline @var tags.
  2. Let FileTypeMapper somehow realize it should cache the whole file including inline @vars in some situations.

@InvisibleSmiley
Copy link

AFAIK the logic for pre-1.0 was that single file analysis results were not cached at all. Why not stick to that simple principle?

(I don't really care either way as long as this gets fixed somehow, just wondering.)

@ondrejmirtes
Copy link
Member

@InvisibleSmiley It's not about result cache, it's about PHPDocs cache.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 11, 2021

If PhpStorm violates the takeaway from this article https://phpstan.org/blog/why-you-should-always-analyse-whole-project then sure, I know the reason why this is happening.

Yes it does. It analyzes only the currently editing file by copying it to tmpdir.

@voku
Copy link
Contributor

voku commented Nov 11, 2021

If PhpStorm violates the takeaway from this article https://phpstan.org/blog/why-you-should-always-analyse-whole-project then sure, I know the reason why this is happening.

Yes it does. It analyzes only the currently editing file by copying it to tmpdir.

Correct: "The problem occurs since we copy files to the temporary folder so the file path becomes /tmp/foobar/<location_of_file> and we use a working dir of the project." -
https://youtrack.jetbrains.com/issue/WI-57630#focus=Comments-27-4604510.0-0

@dktapps
Copy link
Contributor Author

dktapps commented Nov 13, 2021

The more I run into this the more I'm certain that the PhpStorm hypothesis is correct. I ran into this dozens of times yesterday while updating an old project. The whole time I was switching between PhpStorm, and CLI to run PHPStan, and then back again.

@staabm
Copy link
Contributor

staabm commented Nov 13, 2021

See https://youtrack.jetbrains.com/issue/WI-63918 for a similar problem, related to single-file analysis

@dktapps
Copy link
Contributor Author

dktapps commented Nov 13, 2021

Yeah, but running analysis on single files shouldn't break PHPStan this way, regardless of what JetBrains decides to do.

@PHLAK
Copy link

PHLAK commented Nov 15, 2021

I've been running into this recently as well. An interesting observation that may or may not be relevant is that running phpstan clear-result-cache does not fix the problem but manually deleting $TEMP/phpstan does.

@ondrejmirtes
Copy link
Member

Should be fixed: phpstan/phpstan-src@2652f2d

Please test dev-master to confirm the fix. Thanks.

@knallcharge
Copy link

Did a testrun with phpstan then PhpStorm project code inspection then phpstan, no false positives any more, looking good to me!

@dktapps
Copy link
Contributor Author

dktapps commented Nov 24, 2021

Hi, this is still happening on PHPStan 1.2.0. may have been because I was using an older dev version of PHPStan for PhpStorm, will re-test.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 26, 2021

@ondrejmirtes please reopen this, it's still occurring (just ran into it again now, everything is definitely updated this time).

@ondrejmirtes
Copy link
Member

Please open a new issue and specify where it's happening exactly. Try to provide the exact steps to reproduce.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 26, 2021

It's like before, I have no idea what triggers it and it doesn't happen in the same place each time. Sometimes I can go for a day or two without seeing it.

@voku
Copy link
Contributor

voku commented Nov 26, 2021

Same here, PHPStan 1.2.0 and I still see such errors 1-2 times per day. I'm simply manually deleting the tmp files, and then it's working again.

I have added a different phpstan-config for the integration in phpstorm (with a different tmpDir) but I see the error in the cli and in phpstorm, so maybe it still shares some cache?

@dktapps
Copy link
Contributor Author

dktapps commented Nov 30, 2021

Hi, @ondrejmirtes the remaining false-positives I get are all coming from places where @phpstan-var is used. I believe this is the cause of the problem. (Maybe @psalm-var for others too.)

@ondrejmirtes
Copy link
Member

@dktapps Makes sense given the fix: phpstan/phpstan-src@2652f2d

Feel free to send a PR for prefixed tags.

@github-actions
Copy link

github-actions bot commented Jan 1, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants