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

Allow auto imports of type members #378

Open
tgodzik opened this issue Apr 8, 2024 · 8 comments
Open

Allow auto imports of type members #378

tgodzik opened this issue Apr 8, 2024 · 8 comments

Comments

@tgodzik
Copy link
Contributor

tgodzik commented Apr 8, 2024

Is your feature request related to a problem? Please describe.

Currently some libraries such as ZIO have a number of useful types contained in a package object, but those types cannot be imported automatically. It would be possible to enable that.

Describe the solution you'd like

Index types that are contained at toplevel of objects and/or package objects

Describe alternatives you've considered

Import manually

Additional context

This would require making sure that indexing is as effective as it is now and also that we don't gather types, which are not useful for anyone.

Search terms

auto import type zio

@brndt
Copy link

brndt commented Aug 8, 2024

@tgodzik
I could try to implement it if it's not so difficult but I would need some guidance of which classes should I look at as I don't have any experience contributing to Metals.

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 8, 2024

This is related to indexing which is a bit of a complex subject but you're welcome to give it a try! It all depends on ScalaToplevelMtags, which uses only a tokenizer to find potential import candidates. Some recent PR that touched the subject is https://github.com/scalameta/metals/pull/6648/files or scalameta/metals#5623

Especially useful in that case is ToplevelLibrarySuite and ScalaToplevelSuite

@brndt
Copy link

brndt commented Aug 8, 2024

This is related to indexing which is a bit of a complex subject but you're welcome to give it a try! It all depends on ScalaToplevelMtags, which uses only a tokenizer to find potential import candidates. Some recent PR that touched the subject is https://github.com/scalameta/metals/pull/6648/files or scalameta/metals#5623

Especially useful in that case is ToplevelLibrarySuite and ScalaToplevelSuite

Thanks!
I tried to change the logic but I think I didn't do it right. Well, at least auto import of package object members still doesn't work if I try to do it using local snapshot.

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 8, 2024

You can try adding a test with zio to CompletionLspSuite via "libraryDependencies" field in metals.json and then print out what is returned from querying in OnDemandSymbolIndex

@brndt
Copy link

brndt commented Aug 8, 2024

I modified test in CompletionLspSuite by just adding package object and it can't find this suggestion

Screenshot 2024-08-08 at 19 37 26

But as far as I understand this test assumes that all package object members are indexed. Am I wrong? @tgodzik

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 9, 2024

That should be it, but workspace search does some further logic and the compiler also, so somewhere in between this getting lost.

You can check the WorkspaceSymbolProvider's search method and CompilerSearchVisitor in the presentation compiler part.

@brndt
Copy link

brndt commented Aug 9, 2024

It turns out the search works fine, I just had bad logic in test.

Screenshot 2024-08-09 at 22 23 26

So it finds out my class DefinedInD but I guess since it has ".package" postfix it doesn't appear in auto-import suggestion. I was I looking for a place where I can change this behaviour and treat it as a normal object without adding this postfix but didn't find.

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 14, 2024

I looked at your code, but I am not sure what you are trying to do, Wouldn't we need to add another case:

        case TYPE =>
          tpe(name.name, name.pos, Kind.TYPE, 0)

to emitMember? We already look into package object correctly, but type is ommited.

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

No branches or pull requests

2 participants