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

Implement code action for importing missing symbols #1065

Merged
merged 13 commits into from Dec 11, 2019

Conversation

@gabro
Copy link
Member

gabro commented Nov 14, 2019

This is draft PR to validate a possible architecture for code actions (aka refactors).

For now, I'm focusing on code actions that resolve existing diagnostics (i.e. quickfixes), but the architecture could be expanded to more general refactors.

This PR implements a working prototype (although it's a bit of a hack) for the "add missing import" quickfix, which gets triggered by the relevant compiler error.

2019-11-15 11 38 48

If we're happy with the general structure, I can move on with the testing infrastructure for code actions and refining the actual implementation.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 22, 2019

I was thinking about how to make it work properly with the presentation compiler and I think a lot of the actions will need to get data from the presentation compiler, so it would be nice to do it via single interface and maybe create some kind of a query interface?

Would be maybe something like:

...
java.util.List[TextEdit] query(kind: QueryType, filter: Predicate<String, Boolean>) 
...
public enum QueryType{
  FindSymbol, Params
}

Just a thought.

@tgodzik tgodzik mentioned this pull request Nov 29, 2019
@gabro gabro force-pushed the gabro:code-actions branch from 086192a to 14da943 Dec 6, 2019
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Dec 6, 2019

@tgodzik / @olafurpg whenever you have time, I could use a review of the last commit, where I remove the "completions hack" and replace it with an ImportProvider, which loosely follows the implementation of CompletionProvider.

I'm not sure that's the way to go, so I could benefit from some early feedback before I proceed with polishing the implementation.

(PS, I'm surely doing something wrong since the imports include _root_ and the ranges of the text edits are often wrong, but I haven't investigated yet)

@gabro gabro requested review from tgodzik and olafurpg Dec 6, 2019
@gabro gabro force-pushed the gabro:code-actions branch from 14da943 to 4a648c3 Dec 9, 2019
Copy link
Collaborator

tgodzik left a comment

Amazing work @gabro !

I am not super familiar with the logic for finding imports. Overall, from what I see, it looks good! There seem to be some duplicated logic though (?), so for sure I would try to deduplicate it.

/**
* Return the necessary imports for a symbol at the given position.
*/
public abstract CompletableFuture<List<AutoImportsResult>> autoImports(String name, OffsetParams params);

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 9, 2019

Collaborator

Maybe we could do something more general:

public abstract CompletableFuture<List<CodeActionResult>> codeAction(String input, ActionType actionType, OffsetParams params);

enum ActionType{
  Import, Implement, TypeAnnotation
}

otherwise we will be adding a lot of different methods for each action.

This comment has been minimized.

Copy link
@gabro

gabro Dec 10, 2019

Author Member

Good point: I'm torn about this. From one end, I share your concern, from another I'm not sure of what code actions will be implemented so I'm wary of generalizing the API without knowing the use cases.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 11, 2019

Collaborator

Ok, let's leave it then and see if generalizing it will be useful in the future.

gabro added 4 commits Nov 13, 2019
@gabro gabro force-pushed the gabro:code-actions branch from 160ea11 to d04006d Dec 10, 2019
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Dec 10, 2019

I've added some basic tests, that are currently failing due to the issue with the import scope I've mentioned above.

gabro added 4 commits Dec 10, 2019
@gabro gabro marked this pull request as ready for review Dec 10, 2019
gabro added 2 commits Dec 10, 2019
Apparently the range sent for a code action is not stable, but it
depends on the cursor position at the moment of the request.
Therefore, we can't use it to identify the target symbol for
auto-import.

We now use the diagnostic range instead.
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 10, 2019

This is very exciting! I believe this feature may be the most commonly requested feature judging by questions on gitter/discord.

I won't be able to review this PR in depth anytime soon . If you think the implementation is solid enough, then I think it's fine if we merge it nevertheless and we can leave improvements to the future. What do you think?

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Dec 10, 2019

I won't be able to review this PR in depth anytime soon . If you think the implementation is solid enough, then I think it's fine if we merge it nevertheless and we can leave improvements to the future. What do you think?

I think it's decent enough to unblock some more experiments on code actions.

I'll add an LSP test suite, then we can probably merge.

If you have time, can you just comment on the general approach of adding autoImports as a method on the Java interface PresentationCompiler, as opposed to have a more general method for querying the compiler, as proposed by @tgodzik?

Copy link
Collaborator

tgodzik left a comment

Looks very good! Just a few comments, but most importantly I would some unit tests. Might not be super important in this case, but would be cool to have those ready for other code actions.

"""|package a
|
|object A {
| <<Future>>.successful(2)

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 11, 2019

Collaborator

Could you add a case with a partial import?
For example:
<<concurrent.Future>>

This comment has been minimized.

Copy link
@gabro

gabro Dec 11, 2019

Author Member

I don't think that works, since we get a diagnostic on the package name which we can't resolve, it seems. For example:

image

hovering on time won't yield any quick action (no completions are returned either)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Member

this is because the classpath index doesn't support querying packages. It should be an easy fix to implement

This comment has been minimized.

Copy link
@gabro

gabro Dec 11, 2019

Author Member

I imagined. Should we consider that out of scope for this PR?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

It’s best to implement that as a separate pr. I estimate it’s a simple fix

@gabro gabro requested a review from tgodzik Dec 11, 2019
Copy link
Collaborator

tgodzik left a comment

Looks awesome! 🎉 Thanks!

@gabro gabro merged commit f4aefd3 into scalameta:master Dec 11, 2019
11 checks passed
11 checks passed
ubuntu-latest unit tests
Details
windows-latest unit tests
Details
macOS-latest unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@gabro gabro deleted the gabro:code-actions branch Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.