Conversation
For non-aliased imports, emit a local definition on the package name (last segment of the import path), matching the behavior that aliased imports already have. This ensures that usages like modfile.Parse() resolve to the import line rather than the dependency's package declaration, fixing the inconsistency described in #34.
jupblb
force-pushed
the
michal/import-local
branch
from
6941bd8 to
a94dfca
Compare
- Exclude blank imports (import _ "pkg") from local definition handling, as they don't create usable package bindings. - Use the source import literal for range calculation instead of importedPackage.PkgPath, which could theoretically differ.
jupblb
force-pushed
the
michal/import-local
branch
from
a5ad737 to
0abc67a
Compare
Extend the local definition range to span the full import path string instead of just the last segment. This matches the global reference range, giving both occurrences the same span.
Every file's package statement is an equally valid declaration of the package. Previously only one heuristically chosen file got the Definition role while others were marked as ReadAccess. Now all package statements are definitions, with the best file still providing the SymbolInformation documentation.
Covers all import variants: non-aliased (single and multi-segment), aliased, dot imports, blank/side-effect imports, and multiple files in the same package. See #34
Match gopls convention: show 'package config' instead of 'import config github.com/sourcegraph/scip-go/internal/config'.
| // ^ definition local 0 | ||
| // ^^^^^^^ reference github.com/golang/go/src go1.22 testing/ | ||
| // ^ definition local 1 | ||
| // ^^^^^^^ reference local 0 |
There was a problem hiding this comment.
This is where I start second guessing my original request. So I'm going to just think out loud here:
- Really, while this references the local variable
testingindeed, which is defined at the top of the file, that reference is an alias to the external import. - Without the import, this value would not resolve, so in a pure-Go semantic sense, that import is de-facto the definition of the
testingstring. - So effectively, either way is valid:
a. We create a local that is defined within the import, then use that local across the file
b. Or; we pierce the veil, and just mark all instances as references.
Option A means that if we are doing a references lookup from the original package testing statement, we'll get all individual usages of the testing package within the files. -> this is fairly verbose, but accurate
Option B means that if we do references lookup, we'll just get the individual files that reference this package, which in Go will likely mean there is 1 or more uses of this symbol.
If we move this same logic into a Java world (Or TS/JS), where imports are a bit more flexible, I think option A is probably the better option, but harder to achieve (given that in java you may import com.google.guava.*, or in TS you may be importing using a const testing = require("testing") either of these make it hard to pick either one (in Java option A would definitely be preferred, in JS option A would require a lot of custom logic for each package loader)..
I'd say, where possible, import package usages should try to point to the original package where possible, if not, assigning a local and pointing to that is acceptable.
Just needs to be consistent, so if we alias import, it should STILL point to the original package.
There was a problem hiding this comment.
TL;DR: let's revert to the old behavior with the exception of making every package statement a symbol definition.
The way I see it, writing import "net/http" is equivalent to import http "net/http" (a directory/package binding). I think this was probably the original reason why you suggested that the local symbol definition should be restricted to the last part of the import path.1
In Java, writing import java.util.List means that List is a specific symbol defined globally at java.util package. It does intuitively make more sense to me that we always treat it as a global symbol.
Both languages treat packages differently. I don't think we can find a good, universal solution here. The solution suggested in this PR does feel justified in context of the language semantics but I agree this may cause some discrepancy on index consumer end. Which makes me propose the following:
- We should preserve the change where
packagesymbol is marked as definition in every file that contains this statement. - We must preserve the current behavior where
import h "net/http"creates a local symbol definition forh. This is not directly equivalent, but when I create atypealias I do not expect the code index to treat the new definition as reference to the old one. The case ofimportis a bit different but not enough to introduce such inconsistency.
I'm not sure how to proceed with having a local symbol definition for an import statement that is also a global reference. Because of that I think this change should be dropped and we should revert to the original behavior (option B from your message, treating all references as global; with the exception of still treating import aliases as local).
Footnotes
-
This was actually something I implemented in early commits on this branch but I found having local symbol definition and global symbol reference partially overlapping even more confusing. If
net/httpis a global reference andhttpis a local definition, whatever way we decide to handle "find references" it's going to be ultimately different fornetandhttp. ↩
2 participants
For non-aliased imports, emit a local definition on the package name (last segment of the import path), matching the behavior that aliased imports already have. This ensures that usages like modfile.Parse() resolve to the import line rather than the dependency's package declaration.
Fixes #34.