Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indexer: fix #[\Attribute] resolution #2476

Merged
merged 3 commits into from
Dec 28, 2023

Conversation

przepompownia
Copy link
Contributor

@przepompownia przepompownia commented Dec 28, 2023

It's not perfect because

        $this->workspace()->put('project/Bax.php', '<?php use Not\Attribute; #[Attribute] class Bax {}');

still can be considered attribute.

@mamazu
Copy link
Contributor

mamazu commented Dec 28, 2023

In consideration that we might want to do more with attributes in the future we might as well fix it properly. I have seen that for implementations of the class there is a getResolvedName function.
https://github.com/phpactor/phpactor/blob/efc74ee9043e59dd8d1ded5cb6186308fb0e6efc/lib/Indexer/Adapter/Tolerant/Indexer/ClassDeclarationIndexer.php#L96C29-L96C44

Maybe we could use this for attributes and also fix the issue described above with the namespace not being taken into consideration.

@przepompownia
Copy link
Contributor Author

@mamazu thanks for suggestion.

@@ -54,7 +54,7 @@ public function indexAttributes(ClassRecord $record, ClassDeclaration $node): vo
continue;
}
/** @phpstan-ignore-next-line */
if ($attribute->name?->getText() === 'Attribute') {
if ((string) $attribute->name?->getResolvedName() === \Attribute::class) {
Copy link
Collaborator

@dantleech dantleech Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getResolvedName is however potentially enormously inefficient. For this case it's probably enough to just check both variations? cc @mamazu

Copy link
Contributor Author

@przepompownia przepompownia Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dantleech if we check both 'Attribute' and '\Attribute' (and nothing else) then Not\Attribute still remains considered attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. I did not checked how much the indexing time increases after this change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be relative to the numnber of attributes in the source tree i guess

Copy link
Contributor

@mamazu mamazu Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it could be expensive. I haven't thought about it too much and just having a "good enough" solution would probably be sufficient.

I mean the current solution would also not pick up attributes if they have a renaming use statement like so:

use Attribute as PhpAttribute;
use SomeOtherPackage\Attribute;

#[PhpAttribute]
class MyClass{}

Copy link
Contributor Author

@przepompownia przepompownia Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently of the discussion about expensiveness I added such example to the test.

@przepompownia przepompownia changed the title Fix indexing for classes with absolute #[\Attribute] Indexer: fix #[\Attribute] resolution Dec 28, 2023
@dantleech dantleech merged commit 1086986 into phpactor:master Dec 28, 2023
9 of 10 checks passed
@przepompownia przepompownia deleted the fix-abs-attribute-completion branch December 28, 2023 21:24
@dantleech
Copy link
Collaborator

Performance is probably "fine" we already do this for all class references, so a fee more shouldn't hurt.

@przepompownia
Copy link
Contributor Author

Now Symfony Route attribute became available in completion results. Thanks for merging.

@dantleech
Copy link
Collaborator

Oh we might want to bump the indexer version to force reindexing when people upgrade, but no biggie

@mamazu
Copy link
Contributor

mamazu commented Dec 29, 2023

Now Symfony Route attribute became available in completion results. Thanks for merging.

Oh that's why it wasn't showing on my project. Was wondering why. :D Thanks for fixing that (It probably also affects more symfony attributes) like AsContainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants