IndexedReferenceFinder: possibly too wide scope of searching

[przepompownia]

For example, go to lib/CodeTransform/Adapter/WorseReflection/Transformer/ImplementContracts.php and find references to $reflector private property.

Currently as a user I get notifications about scanning ~300 references

I started to debug what happens. At the moment I see that:

1. $reflector is detected as a variable, not a property. It probably needs some fixing.
2. Even assuming that we search for references to a variable I need an explanation why searching is performed outside the class. Not sure if it's worth attention after fixing 1.

[mamazu]

I mean for properties it also would make sense to take the visibility into account. Like searching for a private variable outside of the class makes no sense and searching for protected variables outside of the inheritance hierarchy makes no sense as well.

[przepompownia]

For the record: in class X { function __construct(public $<>y) { print($y); } } $y needs to be considered both property and variable.

[mamazu]

Yeah, but both should have a very limited scope. The variable only in the constructor and the member only in this class.

[dantleech]

Fwiw the legacy reference finder does take visibility into account and is obviously many times faster for private and protected members, it' a missing feature in the "new" system

[przepompownia]

At the moment I give up. I found that we search for property type (as expected) in MemberQuery

but have not idea when and how the list of references should be restricted accordingly to the visibility.

[mamazu]

Okay, I think that might be an indexer problem, it's looking through all references of all files. Which for the $reflector is 218 file references. So renaming the reference to $reflector1 would improve the reference finder.

[mamazu]

@dantleech I have a question because it sounds so obvious:

• When indexing a member we only index (for reference finding) the references with file and offset
• When finding references: we open the file reflect on it and check if the class is in the class hierarchy of our class.

Why don't we also index the context of the reference (for example the class name) into the record? Yes this increases the size of the index but it will drastically reduce the amount of work reference finding takes.

[dantleech]

Yes this increases the size of the index but it will drastically reduce the amount of work reference finding takes

AFAIK we do index the class name (the MemberRecord::$containerType) but we cannot do this when it's not known at compile time. statically analysing the code when creating the index would make the indexing process 1000s of times slower (not to mention that the index can often be inaccurate for various reasons).

[dantleech]

have not idea when and how the list of references should be restricted accordingly to the visibility.

basically you don't need the indexer here, you only need reflection. so refactoring is probably required.

[mamazu]

AFAIK we do index the class name (the MemberRecord::$containerType) but we cannot do this when it's not known at compile time.

I think I'm starting to understand the problem now and I completely agree that static analysis on indexing is a bad idea but maybe we could add a best guess, like if you're accessing $this->prop on class A, then the container type of prop could be A. (It's not perfect in case of inheritance) but it will definitely reduce the amount of results quicker.

[dantleech]

For private and protected the speed can be i increased significantly by not using the indexer at all... which is exactly what the non LSP version does (and it's reliable as doesn't depend on thrnindex to work)