Using generators to get a list of reflections #1476

shcherbanich · 2025-01-05T18:16:07Z

Part of PR #1475 related to using generators instead of arrays to get a list of reflections

What has been done?

locateIdentifiersByType now returns a generator to process each individual class sequentially
reflectAllClasses / reflectAllFunctions / reflectAllConstants methods of DefaultReflector class now return generators instead of arrays
FindReflectionOnLine now takes reflections one by one using generators

…lete

Ocramius · 2025-01-06T03:57:14Z

src/Reflector/DefaultReflector.php

     */
-    public function reflectAllClasses(): iterable
+    public function reflectAllClasses(): Generator


We don't need to change the native type here: iterable is a supertype of Generator, and works fine

Ocramius · 2025-01-06T03:57:30Z

src/Reflector/DefaultReflector.php

@@ -43,11 +44,11 @@ public function reflectClass(string $identifierName): ReflectionClass
    /**
     * Get all the classes available in the scope specified by the SourceLocator.
     *
-     * @return list<ReflectionClass>
+     * @return Generator<ReflectionClass>


We should roll back to iterable<int, ReflectionClass> here

This change is the actual BC break, since list<T> is obviously an extremely small type

Ocramius · 2025-01-06T03:59:14Z

src/Reflector/DefaultReflector.php

     */
-    public function reflectAllFunctions(): iterable
+    public function reflectAllFunctions(): Generator


Same as comment above: the native type can stay iterable, and the return type can be iterable<int, ReflectionFunction>

Ocramius · 2025-01-06T03:59:25Z

src/Reflector/DefaultReflector.php

+     * @return Generator<ReflectionConstant>
     */
-    public function reflectAllConstants(): iterable
+    public function reflectAllConstants(): Generator


Same as comment above: the native type can stay iterable, and the return type can be iterable<int, ReflectionConstant>

Ocramius · 2025-01-06T03:59:52Z

src/Reflector/Reflector.php

-     * @return list<ReflectionClass>
+     * @return Generator<ReflectionClass>
     */
-    public function reflectAllClasses(): iterable;
+    public function reflectAllClasses(): Generator;


This interface should be rolled back, and the @return types should switch from list to iterable

Ocramius · 2025-01-06T04:05:01Z

src/SourceLocator/Type/Composer/PsrAutoloaderLocator.php

     */
-    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): array
+    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): Generator


The implementation of this class can be rolled back, except for the return type being iterable

Ocramius · 2025-01-06T04:05:05Z

src/SourceLocator/Type/DirectoriesSourceLocator.php

-     * {@inheritDoc}
-     */
-    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): array
+    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): Generator


The implementation of this class can be rolled back, except for the return type being iterable

Ocramius · 2025-01-06T04:05:10Z

src/SourceLocator/Type/FileIteratorSourceLocator.php

@@ -83,8 +84,8 @@ public function locateIdentifier(Reflector $reflector, Identifier $identifier):
     *
     * @throws InvalidFileLocation
     */
-    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): array
+    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): Generator


The implementation of this class can be rolled back, except for the return type being iterable

Ocramius · 2025-01-06T04:06:47Z

src/SourceLocator/Type/MemoizingSourceLocator.php

+        $items = [];
+        foreach ($this->wrappedSourceLocator->locateIdentifiersByType($reflector, $identifierType) as $item) {
+            yield $items[] = $item;


The original implementation caches items before returning them: the new implementation caches items after returning them.

I'm unsure about approach here, but it is a subtle change that can lead to uncached results if the consumer of this iterator crashes

Ocramius · 2025-01-06T04:07:22Z

src/SourceLocator/Type/SourceLocator.php

+     * @return Generator<Reflection>
     */
-    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): array;
+    public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): Generator;


We should move to iterable rather than Generator

Ocramius · 2025-01-06T04:08:25Z

@asgrim @kukulich I think the BC break is worth it here: what do you think? Is it OK to move the source locators and API from list<T> to iterable<int, T>?

asgrim · 2025-01-06T06:05:04Z

@asgrim @kukulich I think the BC break is worth it here: what do you think? Is it OK to move the source locators and API from list<T> to iterable<int, T>?

Reasonable change yes; any impact to our own projects we can handle but definitely keen for @kukulich to check here; depending how phpstan consumes these APIs, may or may not be impactful

kukulich · 2025-01-06T06:39:18Z

@ondrejmirtes will this change be a problem for PHPStan?

ondrejmirtes · 2025-01-06T08:11:23Z

@kukulich Yes, it's a BC break. For example I have custom source locator wrappers which do this:

	public function locateIdentifiersByType(Reflector $reflector, IdentifierType $identifierType): array
	{
		return $this->sourceLocator->locateIdentifiersByType($reflector, $identifierType);
	}

If these changes really lead to better performance (so that BR does not do work that would be thrown away), it might be a better idea to introduce new methods or interfaces that return generators, and keep the old classes and interfaces there.

After that we can keep the old methods that will just do iterator_to_array on the new methods. Optionally we could deprecated the "old" methods.

Unfortunately if these changes go through and BetterReflection releases a new major, I could not upgrade to it in PHPStan with a clear conscience until 3.0 because these source locators are part of public PHPStan API. I know that for example Rector depends on them and also creates its own implementations.

shcherbanich · 2025-01-06T08:42:30Z

@Ocramius @asgrim @kukulich Given all the complexities, let me go back to the first implementation, creating new methods (e.g. iterateIdentifiersByType / iterateAllClasses / ... ) and marking the old ones as deprecated, as noted in the comment above. This won't be difficult, and the change can be released safely as soon as possible.

Ocramius · 2025-01-06T08:58:46Z

@shcherbanich let it rest a bit before jumping at it: as you can see, discussion is underway, at least :D

I could not upgrade to it in PHPStan with a clear conscience until 3.0 because these source locators are part of public PHPStan API.

This is a real bummer: stuff like source locators in BetterReflection are marked internal for good reason (their API is squishy), so this is unfortunate.

That said, I fully understand the problem, and will think about it today/tomorrow, before we decide whether/how to proceed.

ondrejmirtes · 2025-01-06T09:17:13Z

Are they really internal? I'm also implementing my own and I'd say it's often useful for projects to write their own. So it might be better to un-internal them.

shcherbanich added 2 commits January 5, 2025 22:09

Using generators to get a list of reflections

fc0231c

Fix source locator caching: only save cache after all iterations comp…

7fd413e

…lete

shcherbanich mentioned this pull request Jan 5, 2025

Adding methods for sequential processing and filtering of class reflections #1475

Open

Ocramius requested changes Jan 6, 2025

View reviewed changes

Ocramius added enhancement BC break labels Jan 6, 2025

Ocramius added this to the 7.0.0 milestone Jan 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using generators to get a list of reflections #1476

Using generators to get a list of reflections #1476

shcherbanich commented Jan 5, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius Jan 6, 2025

Ocramius commented Jan 6, 2025

asgrim commented Jan 6, 2025

kukulich commented Jan 6, 2025

ondrejmirtes commented Jan 6, 2025

shcherbanich commented Jan 6, 2025

Ocramius commented Jan 6, 2025

ondrejmirtes commented Jan 6, 2025

Using generators to get a list of reflections #1476

Are you sure you want to change the base?

Using generators to get a list of reflections #1476

Conversation

shcherbanich commented Jan 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Jan 6, 2025

asgrim commented Jan 6, 2025

kukulich commented Jan 6, 2025

ondrejmirtes commented Jan 6, 2025

shcherbanich commented Jan 6, 2025

Ocramius commented Jan 6, 2025

ondrejmirtes commented Jan 6, 2025