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

fix: fully_qualified_strict_types must honor kind uses #7704

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jan 9, 2024

fix #7652

@mvorisek mvorisek force-pushed the fix_kind_uses branch 2 times, most recently from 1d5e223 to b396a40 Compare January 9, 2024 23:48
$namespaceName = $namespace->getFullName();
$uses = [];
$lastUse = null;
foreach ([self::KIND_CLASS, self::KIND_FUNCTION, self::KIND_CONST] as $kind) {
Copy link
Member

Choose a reason for hiding this comment

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

To be honest I think I prefer my implementation in #7705 because it does not multiply iterations over file. "Everything" is collected and fixed in dedicated phase, because caches are sliced by import kind.

Copy link
Contributor Author

@mvorisek mvorisek Jan 10, 2024

Choose a reason for hiding this comment

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

The performance will be nearly the same in reality.

I actually prefer mine, because there is no point for passing $kind into resolve/shorten type. We should pass only relevant (for relevant kind) uses.

Also filtering for specific kind fixes can be done easier in the future.

Copy link
Member

@Wirone Wirone Jan 10, 2024

Choose a reason for hiding this comment

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

The performance will be nearly the same in reality.

How's that? You iterate over 3 kinds, for each of them you determine namespace declarations, and for each namespace you do 2 phases of processing. That's 9 iterations over tokens list (considering single namespace in file), comparing to 3 in my implementation. For large files it will have significant difference 🙂.

there is no point for passing $kind into resolve/shorten type. We should pass only relevant (for relevant kind) uses.

It is required with cache-per-kind approach, because symbol comparison must be done within the same kind. So even if there's one additional thing to pass around, it's much less overhead than 3 times more iterations over same tokens list.

Also filtering for specific kind fixes can be done easier in the future.

What kind of filtering? You mean "fix only functions"? Probably that if tree should be sliced by kind, I don't see as a problem in my implementation 😅.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Iterating million of tokens should not be that slow. Do you have some numbers?

But you PR is fine as well, maybe get inspired by self::KIND_ constants and cherry pick the 2nd refactor, then the PRs outside the main loop are basically the same.

Also feel free to impl. the actual function/const locating, I do not plan to implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/fqcn Fully Qualified Class Name usage and conversions
Projects
None yet
2 participants