DEVSENSE / phptools-docs

PHP Tools public content
Apache License 2.0
80 stars 10 forks source link

Find all references works wrong with public methods #322

Open AndreiOrmanji opened 1 year ago

AndreiOrmanji commented 1 year ago

Descrition: I've created an interface A with 1 method process and 1 implementation (class B) and one class that has a method with the same name but it does not implement interface A Example from description: image

Expected result: No references found for method C::process()

Actual result: All methods with the same name are returned, even when signature does not match.

If this issue will result in PR with fix, please check it at least with my code:

<?php
interface A
{
    public function process(): void;
}

class B implements A
{
    public function process(): void
    {
        echo 1;
    }
}

class C
{
    public function process(): void
    {
        echo 2;
    }
}

Expected result for all methods process: 1) "Find all references" on A::process() should return reference on itself and B::process() 2) "Find all references" on B::process() should return reference on itself and A::process() 3) "Find all references" on C::process() should return reference on itself only

Environment details: Extension version: v1.33.12934 VSCode: 1.77.3 (running in WSL via Remote Development package on distro Ubuntu 20.04.6 LTS) OS: Windows 10 build 19043.2364

jakubmisek commented 1 year ago

Thank you @AndreiOrmanji for the detailed issue!

I'll try to replicate it and fix it!

AndreiOrmanji commented 1 year ago

@jakubmisek It seems the problem exists for a long time. https://community.devsense.com/d/697-find-all-references-works-wrong/11

jakubmisek commented 1 year ago

@AndreiOrmanji Thank you for your issue. We'll be looking into it.

jakubmisek commented 1 year ago

Preparing a pre-release, that fixes most of the find all references inaccuracies.

AndreiOrmanji commented 1 year ago

@jakubmisek I've installed today pre-release version to check and the extension still finds wrong references. For some reason Find all references for C::process() returns references from vendor of non-related methods, but with the same name. image

Description of fixes says "Find All References performs type analysis if necessary to provide better results". Why does "find all references" not use ONLY type analysis? I can not say for anyone else but for me is important to get a list of references that are 100% related to symbol, i'm searching. Other cases that can not be found by type analysis should be searched manually, or a new feature should be introduced to show "less relevant" variants (with the name "find weak references" or something like that).

jakubmisek commented 1 year ago

@AndreiOrmanji thank you for testing it out so quickly.

You're right;

There still seems to be a bug related to untitled documents and/or phar files. I'll keep working on it.

AndreiOrmanji commented 1 year ago

@jakubmisek It's not limited to untitled and phar files. Example:

     $result->{$prop} = is_a($attr, A::class, true) ? (new $attr())->run($step) : $attr;

I have 2 interfaces(A and B) with method run and nor A extends B, nor B extends A and for BOTH of them this row will be shown as a reference.

I did not tried if the extension takes into account arguments' count (but I'm sure that argument type is not taken into account)

jakubmisek commented 1 year ago

@AndreiOrmanji it seems we don't handle is_a() yet - I'll do that, then (new $attr)->run might get resolved correctly

AndreiOrmanji commented 1 year ago

@AndreiOrmanji it seems we don't handle is_a() yet - I'll do that, then (new $attr)->run might get resolved correctly

The main question here is why extension thinks about all objects without specified type that they are of ALL TYPES AT ONCE instead of MIXED?

I would like not to see places where run method is called on an object of unknown type as if it was called by a specific type with run method defined in it.

jakubmisek commented 1 year ago

@AndreiOrmanji I agree - I'll try to fine-tune it a little.

AndreiOrmanji commented 1 year ago

@jakubmisek, It seems that huge amount of work is done to improve the "find all references" feature. Thank you and all your team for your work. Currently (extension version 1.37.13534) for the same code provided in issue description returns the following output for case 3 ("Find all references" on C::process()): image

Can it be adjusted to fix this case?

cDonut commented 11 months ago

find all references still returns incorrect results, it's the only thing that stops me from using this extension

jakubmisek commented 11 months ago

@cDonut thank you for reminding me!