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

Use PsiReference list to determine if "missing" inspections apply #2256

Merged

Conversation

adrolter
Copy link
Contributor

@adrolter adrolter commented Dec 2, 2023

Instead of using static signature lists to check if the route, service, and template "missing" inspections apply, we can instead check if the element's PsiReference list contains the relevant reference type. This change allows these inspections to operate in the same contexts as auto-completion, such as when the reference was tagged with a PhpDoc hash (e.g., #Route) or configured through the settings panel.

Thanks!

Fixes #2255

@Haehnchen Haehnchen merged commit 7e953d1 into Haehnchen:master Dec 15, 2023
1 check passed
@Haehnchen
Copy link
Owner

👍

@adrolter
Copy link
Contributor Author

Thank you!

@adrolter adrolter deleted the feature/inspections-use-psi-refs branch December 15, 2023 22:31
@adrolter
Copy link
Contributor Author

@Haehnchen Looks like this patch is causing a "too many element types registered" issue, as you've also discovered per your revert. Do you have any insight as to why my approach resulted in superfluous registration of IElementTypes? I'm assuming I need to be doing some caching that I'm not, so I'll reimplement properly once I understand where I went wrong.

@Haehnchen
Copy link
Owner

thank for reaching out, but i would better leave it as it was:

  • the many issue looks the removal of the method checks, which filter the possible code triggering
  • references are provide an target, which dont needed in inspections, there is just a bool flag needed (which why the code behind is different and optimized)
  • it looks like the references are fetched live for each possible char typed, resolving it 100 of times; so lot of hidden external references.

too many element types registered

i dont think its directly related, to this changes. but i still need to check, as i also had problems on daily basis on a project. for now guessing that it was called too often, too frequently for "bigger files". I hade problems writting queryies in repo classes and exception came from template checking. :)

Maybe there were changes in PhpStorm itself on lexer / ast node, in which maybe a route name is resolved to an object and cached, but i found no reproducible way, so i reverted the changes for the next release as a first step.

@adrolter
Copy link
Contributor Author

adrolter commented Jan 14, 2024

Understood, but then what is to be done about the "missing" inspections not recognizing custom references? With this revert my team and I are going to lose all visibility of broken routes in our project, for instance, and I don't know what I'm going to do about that now.

Is it not expected that custom references will be inspected? Are people supposed to know they're making a tradeoff when using custom references that will lead to broken references all over their code over time?

Thanks!

@adrolter
Copy link
Contributor Author

@Haehnchen not to be a pain, but could you please advise so that I can determine how to proceed? Thank you

@Haehnchen
Copy link
Owner

@adrolter i will add this in the next release, in the end the "configured reference methods" should be considered. mainly it should just extend the already arraylist.

@adrolter
Copy link
Contributor Author

adrolter commented Jan 24, 2024 via email

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.

Inspections don't use Method References configuration
2 participants