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

x/tools/gopls: redundant import name added #35397

Closed
muirdm opened this issue Nov 6, 2019 · 6 comments
Closed

x/tools/gopls: redundant import name added #35397

muirdm opened this issue Nov 6, 2019 · 6 comments
Labels
Milestone

Comments

@muirdm
Copy link

@muirdm muirdm commented Nov 6, 2019

Steps to reproduce:

Starting with:

package main

import "os/exec"

func main() {
	exec.CommandContext()
}
  1. Complete at CommandContext(<>) and select "context.Background"
  2. I expect to get an import for "context", but I get context "context" (redundant alias).

alias

Also note that it says from "context" in the candidate description although it is not required in this case.

/cc @stamblerre @heschik

@gopherbot gopherbot added this to the Unreleased milestone Nov 6, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 6, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 6, 2019

Change https://golang.org/cl/205657 mentions this issue: internal/lsp/source: don't unconditionally name imports

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Nov 6, 2019

Silly mistake, thanks for the report. Why do you say that from "context" isn't necessary? It's adding a new import and I think the user should know what it is. They could have multiple context packages, or be missing the one they actually want.

@muirdm

This comment has been minimized.

Copy link
Author

@muirdm muirdm commented Nov 6, 2019

Why do you say that from "context" isn't necessary?

In this case I happen to know that I'm getting completions from the package of the function parameter (i.e. "context"), so there is only one possibility. But you bring up a good point that there is no existing import so the user normally can't/won't know where the candidates are coming from.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Nov 6, 2019

That would be true in some cases, but not this one. context.Context is an interface, so I was also getting completions from x/net/context, for example. I think it's probably healthy to show it unconditionally, FWIW.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 7, 2019

Change https://golang.org/cl/205738 mentions this issue: internal/lsp/source: don't unconditionally name imports

gopherbot pushed a commit to golang/tools that referenced this issue Nov 7, 2019
In CL 205501 I thoughtlessly set import name to package name, but really
we only want to name imports when goimports would do it. For now, it's
better to not name them and let the usual imports code add a name if
necessary.

Fixes golang/go#35397.

Change-Id: Id0df866f95e5e86ed72b25fbd1a7224c79ee8084
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205657
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
(cherry picked from commit c2ac6c2)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205738
Reviewed-by: Heschi Kreinick <heschi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.