FNSR ExpressionResultStorage #4628
Conversation
ondrejmirtes
force-pushed
the
fnsr-storage
branch
4 times, most recently
from
c0b8d16 to
8e54658
Compare
|
issue bot results look outstanding - too good to be real 🥸 |
|
Lots of diffs doesn't mean lots of bugs fixed 😀 The analysis is still too wrong. |
…ions to Scope debug
…lready stored expr result
ondrejmirtes
force-pushed
the
fnsr-storage
branch
3 times, most recently
from
895ce9e to
412cc4e
Compare
|
Alright, so this issue-bot report is far more realistic and closer to the end results. Solving issues like https://phpstan.org/r/8d3536cc-d647-455c-8919-300cc35d7abf and https://phpstan.org/r/59b9367b-8f29-4008-8536-91dca00362df is what I had in mind. |
|
I’m sure it’s used. It overrides an empty method from NodeScopeResolver.
Ondřej Mirtes
…On Fri 19. 12. 2025 at 7:30, Markus Staab ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Analyser/Fiber/FiberNodeScopeResolver.php
<#4628 (comment)>:
> + $request->scope->toMutatingScope(),
+ $storage,
+ new NoopNodeCallback(),
+ ExpressionContext::createTopLevel(),
+ );
+ if ($storage->findResult($request->expr) === null) {
+ throw new ShouldNotHappenException(sprintf('processExprNode should have stored the result of %s on line %s', get_class($request->expr), $request->expr->getStartLine()));
+ }
+ $this->processPendingFibers($storage);
+
+ // Break and restart the loop since the array may have been modified
+ return;
+ }
+ }
+
+ protected function processPendingFibersForRequestedExpr(ExpressionResultStorage $storage, Expr $expr, ExpressionResult $result): void
this method seems unused
—
Reply to this email directly, view it on GitHub
<#4628 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZTOFVYCLK67LVP4DOC2L4COLQPAVCNFSM6AAAAACOW6DOEWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKOJXGA4DMMJXG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| } | ||
| } elseif ($expr instanceof Expr\Empty_) { | ||
| $this->processExprNode($stmt, $expr->expr, $scope, $storage, new NoopNodeCallback(), $context->enterDeep()); |
There was a problem hiding this comment.
why/which expressions need to be processed twice? I can see this only happens for a few ones
There was a problem hiding this comment.
Yeah, this was tricky. The problem is that the new system is so annoyingly precise it actually sometimes prevents a rule what it wants to do. When doing $scope->getType($expr), it actually no longer matters what $scope is. It just waits for NodeScopeResolver to reach and process $expr before giving us the result.
This is best explained on NullsafePropertyFetch and NullsafeMethodCall which also need to do this. NullsafePropertyFetchRule needs to report when doing $var?->doFoo() on non-nullable $expr. But with this code in NodeScopeResolver:
$exprResult = $this->processExprNode($stmt, new PropertyFetch(
$expr->var,
$expr->name,
array_merge($expr->getAttributes(), ['virtualNullsafePropertyFetch' => true])
), $nonNullabilityResult->getScope(), $storage, $nodeCallback, $context);The $expr->var is always non-nullable so the rule cannot do its job.
That's why I decided to have different Expr instances for when you have a rule for NullsafePropertyFetch (the original node) and when you have a rule for a synthetic new PropertyFetch. So both NullsafePropertyFetch rules (var is still nullable) and PropertyFetch rules (var is no longer nullable) can do their job.
There was a problem hiding this comment.
Before FNSR this wasn't an issue because the $scope passed to a rule had different types so the rules worked fine even when $var was the same instance.
| */ | ||
| public function cloneNode(Node $node): Node | ||
| { | ||
| $traverser = new NodeTraverser(new CloningVisitor()); |
There was a problem hiding this comment.
I can see we sometimes clone nodes which have already been cloned.
(so we clone the clone)
it might make sense to prevent some unnecessary per node work on this area
There was a problem hiding this comment.
it seems occasionally we even clone the cloned clone.
There was a problem hiding this comment.
I can imagine this might happen on nested calls like $this->foo?->bar?->doBar(). And it's not really preventable.
|
@staabm Alright, I have some performance profiles you could take a look at. Here's 2.1.x: https://blackfire.io/profiles/86fcac97-986a-4e41-92e0-b41aaf53fbcc/graph Right now it's slower because of these reasons. Maybe some optimizations could make it faster. Feel free to send PRs against fnsr-storage, thanks :) What I will personally look into is to how take advantage of ExpressionResultStorage to not process AST nodes multiple times, like it's done for BooleanAnd+BooleanOr in MutatingScope, and also for closure bodies. Also I think we could maybe get rid of these properties in MutatingScope, at least when FiberScope is involved: /** @var Type[] */
private array $resolvedTypes = [];
/** @var array<string, self> */
private array $truthyScopes = [];
/** @var array<string, self> */
private array $falseyScopes = []; |
|
Here's comparison |
…when FNSR is enabled)
This reverts commit d83934e.
ondrejmirtes
force-pushed
the
fnsr-storage
branch
from
412cc4e to
2226259
Compare
|
The build now should be green without any changes, just consuming a bit more memory on large files. I'm going to merge this, then I'm going to clean up the Generator namespace which is no longer going to be developed, then we're going to try to address the downsides of the updated implementation. |
|
Once the build with |
so the idea is, since we already have a caching with |
|
Yeah, something like that. Of course I could be wrong. For example I was first investigating excessive memory usage because of |
2 participants
No description provided.