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: unimported imports don't work with empty selector #35915

Open
muirdm opened this issue Nov 30, 2019 · 12 comments
Open

x/tools/gopls: unimported imports don't work with empty selector #35915

muirdm opened this issue Nov 30, 2019 · 12 comments
Assignees
Labels
Milestone

Comments

@muirdm
Copy link

@muirdm muirdm commented Nov 30, 2019

In code like:

package main
func main() {
  context.
}

I expected to get candidates from "context" but I get nothing.

/cc @heschik

@gopherbot gopherbot added this to the Unreleased milestone Nov 30, 2019
@gopherbot gopherbot added the Tools label Nov 30, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 30, 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 gopherbot added the gopls label Nov 30, 2019
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 2, 2019

Looks like more your bag, it's failing during parsing and never running any completion code at all, I think?

emptysel.go:5:1: expected selector or type assertion, found '}' (and 2 more errors)

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Dec 2, 2019

I don't think so, because we ignore errors in parsing as long as we get an AST back. I think regular completions work fine in this case.

@muirdm

This comment has been minimized.

Copy link
Author

@muirdm muirdm commented Dec 2, 2019

Completion in general works in this case since you get a parser error, but you also get a best effort *ast.File. The root of the issue seemed to be in imports.ApplyFixes(). It does not handle files with syntax errors since it needs to format the entire file. I wasn't sure if the right approach was to trim to just the imports before calling imports.ApplyFixes, or some change under imports/.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 2, 2019

You're both right, of course. CL mailed.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 2, 2019

Change https://golang.org/cl/209579 mentions this issue: internal/imports: make ApplyFixes work despite syntax errors

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 6, 2019

I'm noticing what seems to be the same problem if the empty selector is right before a keyword on the next line.

expected selector or type assertion, found 'if' (and 13 more errors)

@heschik heschik reopened this Dec 6, 2019
@heschik heschik self-assigned this Dec 6, 2019
@muirdm

This comment has been minimized.

Copy link
Author

@muirdm muirdm commented Dec 6, 2019

Do you have a repro? It seems to work normally for me with a keyword on the next line.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 6, 2019

Yeah. Using gopls built at 7a2a8a, inserting base64. at line 207 or on a new line after 211 of format.go causes a parse error. https://golang.org/cl/c/210200/1 fixes the bug because it usually doesn't need to parse the whole file anymore, so maybe it doesn't matter so much.

@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 10, 2019
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 17, 2019

I think the basic gist of this is that I'm not using cache.parseGo, so I'm not getting AST fixups. Maybe I should be?

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Dec 17, 2019

I don't think any of the AST fixups touch the import spec, but maybe it's worth trying this out?

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 17, 2019

At the time I had the problem, I was always parsing the whole file. Now I only need to do that when there's no imports yet, but I imagine the problem would still happen.

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.