scalameta / metals-feature-requests

Issue tracker for Metals feature requests
37 stars 4 forks source link

Allow auto imports of type members #378

Open tgodzik opened 7 months ago

tgodzik commented 7 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 https://github.com/scalameta/metals/pull/5623

Especially useful in that case is ToplevelLibrarySuite and ScalaToplevelSuite

brndt commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.