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

godef: no declaration found for (symbol) when package name != containing directory name #18

Closed
colonelpanic8 opened this issue Oct 31, 2015 · 11 comments

Comments

@colonelpanic8
Copy link

No description provided.

@zippoxer
Copy link

+1

Is it idiomatic to walk through the import paths and visit some go file in each package to get it's name?

tbg added a commit to tbg/opentracing-go that referenced this issue Feb 14, 2016
The implicit import name makes resolution easier for
tools like `godef`, which at the time of writing do
not have the capability to resolve nonstandard
import paths, for example see

    rogpeppe/godef#18

I looked into adding it for `godef`, but it did not
seem trivial.
@tbg
Copy link

tbg commented Feb 14, 2016

I looked into this and it requires a lot of familiarity with how AST parsing/resolution works. With a little guidance, maybe this can be picked up. What I tried naively was to walk through pkg.Imports and seek out any Decl[i].(_GenDecl).Specs[j].(_ImportSpec).Name == nil (in which case we want to load the package given under <same>.Path). I have no idea whether that's the right thing to do. It seems too manual, like there should be a resolver tool that does this already.

@colonelpanic8
Copy link
Author

I think that the problem is with ImportPathToName

func ImportPathToName(p string) string {

This thing should really be going into the path specified in the import and figuring out what the name of the package contained in that path is.

tbg added a commit to opentracing/basictracer-go that referenced this issue Feb 20, 2016
The implicit import name makes resolution easier for
tools like `godef`, which at the time of writing do
not have the capability to resolve nonstandard
import paths, for example see

    rogpeppe/godef#18

I looked into adding it for `godef`, but it did not
seem trivial.
colonelpanic8 added a commit to colonelpanic8/godef that referenced this issue Feb 22, 2016
Fixes rogpeppe#18. This is a work in progress/proof of concept implementation.
@colonelpanic8
Copy link
Author

@tschottdorf I fixed this. Give the version from my pull request a try and see if it works for you.

@colonelpanic8
Copy link
Author

Is it idiomatic to walk through the import paths and visit some go file in each package to get it's name?

I feel like there should be.

@rogpeppe this solution feels pretty hacky/gross, which is why I didn't really bother to clean it up much in my PR. Let me know if you would be willing to accept something like this -- If so I'll clean it up and write a test or two.

@colonelpanic8
Copy link
Author

Also @rogpeppe why does godef seem to have its own version of the go parser/ast code?

@colonelpanic8
Copy link
Author

It looks like #3 was another (much simpler) attempt to solve this issue. What happened to that PR.

@rogpeppe
Copy link
Owner

godef has its own version of the go parser code because when it was written,
I hoped that the changes (that resolve almost all symbols at parse time)
might make it upstream. They didn't, but the fork lives on.

The reason for this bug is essentially that I wanted godef to be fast, so it takes
a shortcut which makes it wrong some of the time. That was probably not
the best decision, but it's quite a hassle to change, and probably not
worth the effort now that guru is almost there.

For the record, the speed is gained because godef resolves
all symbols without actually pulling in the import packages, so
when searching for a local symbol, it only needs to read the
local package's files. This is wrong in two cases: when an imported
package uses a different symbol from its directory name, and
when using "import .".

@colonelpanic8
Copy link
Author

@rogpeppe godef is still plenty fast with this change though. I set the only parse package mode so it should really just open ONE file in the package and read what the package is. I refuse to believe that that has enough of a performance impact to make this change unviable. At the very least, I'd like to see profiling that indicates that it has too large an effect.

Premature optimization is the root of all evil.

@colonelpanic8
Copy link
Author

@rogpeppe I hand't heard of go guru before. I just got it set up and it seems to work really well. What is merely 'almost there' about it?

@rogpeppe
Copy link
Owner

Fixed by #34

Stebalien pushed a commit to ipfs/go-log that referenced this issue Apr 20, 2018
The implicit import name makes resolution easier for
tools like `godef`, which at the time of writing do
not have the capability to resolve nonstandard
import paths, for example see

    rogpeppe/godef#18

I looked into adding it for `godef`, but it did not
seem trivial.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants