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

Compute 'Import missing symbol' code actions for all diagnostics in range #1286

Merged
merged 5 commits into from Jan 11, 2020

Conversation

@gabro
Copy link
Member

gabro commented Jan 11, 2020

Fixes #1283

Previously we computed the 'Import missing symbol' code action for a single diagnostic under the cursor. Now we compute all diagnostics whose range overlap with the code action range sent by the client. This allows computing auto-imports for all diagnostics in a range at once.

This is especially relevant for clients like Vim, which - by default - sends a code action request for the entire line.

Here's how it looks like in VS Code:

2020-01-11 17 07 56

gabro added 3 commits Jan 11, 2020
Bloop wouldn't compile otherwise
@cb372

This comment has been minimized.

Copy link
Contributor

cb372 commented Jan 11, 2020

Nice! Thanks for the, ahem, quick fix.

@@ -46,7 +46,7 @@ class ImportMissingSymbol(compilers: Compilers) extends CodeAction {
Future
.sequence(params.getContext().getDiagnostics().asScala.collect {
case d @ ScalacDiagnostic.SymbolNotFound(name)
if d.getRange().encloses(params.getRange().getEnd()) =>
if params.getRange().overlapsWith(d.getRange()) =>

This comment has been minimized.

Copy link
@gabro

gabro Jan 11, 2020

Author Member

we cannot use encloses because sometimes the code action range is smaller than the diagnostic range, for example:

val x = Future.successful(2)
//      ^^^^^^  diagnostic range
//       <-->   code action range

or even partially overlapping

val x = Future.successful(2)
//      ^^^^^^  diagnostic range
//         <------->   code action range
val input = m.Input.String(text)
val path = root.resolve(filename)
path.touch()
val pos = m.Position.Range(input, startOffset, endOffset - "<<>>".length())

This comment has been minimized.

Copy link
@gabro

gabro Jan 11, 2020

Author Member

the endOffset is adjusted to account for the removal of the range markers

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jan 12, 2020

Collaborator

A lot of this code seem duplicate to what we already have. Would it be possible to reuse the methods that we use for changing to the position in params?

This comment has been minimized.

Copy link
@gabro

gabro Jan 14, 2020

Author Member

@tgodzik which method? Anyway, sure, I'll open a tech debt issue.

Copy link
Member

ckipp01 left a comment

So my initial thought is that this looks great! Nice job! @cb372 beat me to the quick action pun 😉 .

However, I pulled it down and am trying it out, and I'm a bit confused why this isn't working. For example, here is the trace.

[Trace - 05:29:33 PM] Received request 'textDocument/codeAction - (6)'
Params: {
  "textDocument": {
    "uri": "file:///Users/ckipp/Documents/scala-workspace/test-project/example/src/Main.scala"
  },
  "range": {
    "start": {
      "line": 1,
      "character": 0
    },
    "end": {
      "line": 2,
      "character": 0
    }
  },
  "context": {
    "diagnostics": [
      {
        "range": {
          "start": {
            "line": 1,
            "character": 10
          },
          "end": {
            "line": 1,
            "character": 16
          }
        },
        "severity": 1,
        "source": "bloop",
        "message": "not found: value Future"
      },
      {
        "range": {
          "start": {
            "line": 1,
            "character": 28
          },
          "end": {
            "line": 1,
            "character": 35
          }
        },
        "severity": 1,
        "source": "bloop",
        "message": "not found: value Instant"
      }
    ]
  }
}


[Trace - 05:29:33 PM] Sending response 'textDocument/codeAction - (6)'. Processing request took 11ms
Result: null

I actually can't get a code action to trigger at all anymore, even with a selection on a single diagnostic. I'm trying to figure out why this is happening, but I'm a bit confused.

@cb372

This comment has been minimized.

Copy link
Contributor

cb372 commented Jan 11, 2020

@ckipp01 I just tried it in vim and it works like a charm for me.

Screenshot 2020-01-11 at 16 36 14

EDIT: This was from calling :CocAction while the cursor was anywhere on the line val x = Future(1).

@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Jan 11, 2020

Hmmm, ignore my comment I guess. Something is all screwed up with my local build. I can't even get it to work with VS Code either, and I'm getting a bunch of exceptions.

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Jan 11, 2020

FWIW I can't get it work in nvim either (I haven't tried vim yet), but it works in VSCode...

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Jan 11, 2020

Scratch that, I was using an old version.

I can confirm I got this working on nvim with coc-metals too.

image

if (initializeParams.supportsCodeActionLiterals) {
capabilities.setCodeActionProvider(true)
} else {
capabilities.setCodeActionProvider(
new CodeActionOptions(
List(CodeActionKind.QuickFix, CodeActionKind.Refactor).asJava
)
)
} else {
capabilities.setCodeActionProvider(true)
}
Comment on lines 509 to 517

This comment has been minimized.

Copy link
@gabro

gabro Jan 11, 2020

Author Member

I got this wrong in #1251. When the client supports code action literals we can send the CodeActionOptions version, otherwise we just send true

Copy link
Member

ckipp01 left a comment

Alright, I got my issues worked out, and indeed, this works great now @gabro! I say LGTM!

Also, maybe this can lead to a "Import all missing Imports" for a selection range 😏

@gabro gabro merged commit 1faa832 into scalameta:master Jan 11, 2020
12 checks passed
12 checks passed
windows-latest jdk-11 unit tests
Details
macOS-latest jdk-11 unit tests
Details
ubuntu-latest jdk-8 unit tests
Details
ubuntu-latest jdk-11 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.