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

Add EP to customize "Add Import" weights #2853

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

Conversation

andymagee
Copy link
Contributor

@abelkov abelkov added the kotlin Pull requests for Kotlin IntelliJ plugin label Oct 7, 2024
@andymagee
Copy link
Contributor Author

Friendly ping on this - any updates?

@fedochet
Copy link
Member

Hi @andymagee !

We actually already have a simlar EP for callables - KotlinAutoImportCallableWeigher

Could you please see if it is possible to extend it or introduce a new EP which would look similar to it?

Fix for https://youtrack.jetbrains.com/issue/KTIJ-31471

KotlinAutoImportCallableWeigher allows customization of the order within
the auto import list. This change updates it to more accurately handle
imports for type names.
@andymagee andymagee force-pushed the kotlin_import_fix_weight branch from b8a6875 to c3b4bf4 Compare October 24, 2024 18:05
@andymagee
Copy link
Contributor Author

andymagee commented Oct 24, 2024

Hi @andymagee !

We actually already have a simlar EP for callables - KotlinAutoImportCallableWeigher

Could you please see if it is possible to extend it or introduce a new EP which would look similar to it?

Thank you, I had missed that existing EP!

The existing weigher uses CallExpressionWeigher/CallExpressionImportWeigher for all name references, even though the name references are not always call expressions (eg, in the case of type name references like val foo: SomeUnimportedType). I've updated the logic to more accurately handle type names that aren't callables, and added a number of test cases to validate those.

I'm also removing the @Internal markings for the EP, since the goal would be to use this in the Android Studio codebase.

Let me know what you think, thanks.

There's an existing test for TypeAlias import ordering, but it's only
passing by accident. Adding a single parameter to the annotation that's
being aliased causes the order to get swapped, which doesn't match the
expected behavior. (I'm assuming the expected behavior is that the
typealias should always be lower on the list; this is based on the
existing test case.)

The existing test case worked because the annotation had a default
constructor with 0 arguments, and the referencing location that needs an
import also has 0 arguments. But any time that argument count doesn't
match, then the order is no longer determined by this weigher.

This fix simply demotes any TypeAlias elements slightly.
@andymagee
Copy link
Contributor Author

Hi there, any comments on the updated changes?

@fedochet
Copy link
Member

Sorry for the delay, I will take a look at it this week

@andymagee
Copy link
Contributor Author

Hi @fedochet , have you had a chance to take a look at this PR?

@fedochet
Copy link
Member

Hi again Andy!

I have looked at your PR and the original issue once again, and I see that in the AS you've previously used ComposeProximityWeigher to achieve the same goal, and then it stopped working at some point

Unfortunately, I am not sure that I am (as the owner of the auto-import subsystem) ready to make the KotlinAutoImportCallableWeigher EP public in the current state - it definitely needs some extra-work IMO

My main concern is that in the current form the EP seems a little bit clumsy, and it is also somewhat under-used in our own code. Another concern is that it is not related to weighers, which seems like an overlooking from our part

If it's possible, I would like to postpone this PR for some time. I am currently woking on auto-import subsystem, and I would also pay attention to the existing EPs, and will take your paricual problem (https://youtrack.jetbrains.com/issue/KTIJ-31471) into consideration as well

WDYT?

@andymagee
Copy link
Contributor Author

That sounds like a reasonable plan, and I agree that it makes sense to have a single mechanism rather than multiple different types of weighers.

Do you have any timeline in mind for this work? Depending on when we would expect any updates, we may need to make some workarounds on our side for this in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kotlin Pull requests for Kotlin IntelliJ plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants