NodeScopeResolver: Prevent repetitive union of static types

[staabm]

before PR

---

after PR - union no longer appears as one of the callees

[ondrejmirtes]

Would be better if it was lazily initialized with ??= right before usage.

[staabm]

I think the NodeScopeResolve is created once for the whole analysis and its pretty likely that we hit the path on usage.

so I think there is not much to be saved. only difference would be readability, as we would move code more to a single place (but we will also loose readonly attribute)

[staabm]

so its a matter of code readability, not performance.

if you prefer I can move it to a ??=

[ondrejmirtes]

I worry about doing things in constructor. For example if NodeScopeResolver depended on ReflectionProvider or vice-versa, doing something related to Type objects in constructor could lead to infinite recursion.

[staabm]

good points, didn't think about that.

[ondrejmirtes]

Thank you!

[VincentLanglet]

Maybe I'm missing something but can't the TypeCombinator::union be replace by a new UnionType( call ?

If not and if we're starting caching some often used Union to reduce the number of call of TypeCombinator::union would it be useful/better to have those in a dedicated class rather than starting adding static property to multiple classes (in the future) ?

[ondrejmirtes]

It's true it could be new UnionType, but this will be literally a single call during the analysis so it's not going to cost us any time.

[VincentLanglet]

It's true it could be new UnionType, but this will be literally a single call during the analysis so it's not going to cost us any time.

I thought that without the need of using TypeCombinator::union and just a new UnionType instead we could also avoid a dedicated property (cause there no need to avoid this constructor call isn't it) and save some memory (even if I assume it's small).
Cause if the goal is even to save new *Type calls, we'll end with private property everywhere...

But I'm not familiar at all with perf/memory costs and never work to improve a codebase about it, so I rely on your and staabm's experience.

[staabm]

I thought that without the need of using TypeCombinator::union and just a new UnionType instead we could also avoid a dedicated property (cause there no need to avoid this constructor call isn't it) and save some memory (even if I assume it's small).

new UnionType([...]); creates a non-normalized union type, which means later calls to TypeCombinator::* need to normalize it.

TypeCombinator::union() returns a normalized union type, which means we also move this repeated cost of normalization to the construction time and do it only once instead of every time.

Cause if the goal is even to save new *Type calls, we'll end with private property everywhere...

I wouldn't say the goal is to save all the new *Type calls. this change was motivated by a blackfire profile which showed produceArrayDimFetchAssignValueToWrite is the slowest consumer of TypeCombinator::intersect().

the change was done to get rid of a hot path. it does not mean similar pattern in other less-hot path should be treated the same.