Add goto_definition() support for :: and :::#1332
Conversation
| pub enum PackageVisibility { | ||
| Exported, | ||
| Internal, | ||
| } |
There was a problem hiding this comment.
This file and identifier.rs need to share a single NamespaceVisibility type
I renamed it from PackageVisibility to NamespaceVisibility because it feels like identifier.rs is a better owner of this type, and there we have Identifier::NamespaceAccess as the struct that owns the NamespaceVisibility field, so that name felt better.
| /// When we know `pkg` (a workspace or installed package in the db), resolve | ||
| /// `name` against it and run the path shared with bare variables. We resolve | ||
| /// with `Internal` visibility because `::` and `:::` name the same binding for | ||
| /// a references search, so we want the definition whether or not it is | ||
| /// exported. | ||
| /// | ||
| /// When `pkg` is unknown (e.g. an installed package not loaded into the db), | ||
| /// there is no definition to compare against, so fall back to a structural scan | ||
| /// that matches `pkg::name` sites by text. | ||
| fn find_namespace_references<'db>( | ||
| db: &'db dyn Db, | ||
| primary: File, | ||
| namespace: Name<'db>, | ||
| package: Name<'db>, | ||
| name: Name<'db>, | ||
| include_declaration: bool, | ||
| ) -> Vec<FileRange> { | ||
| if let Some(package) = db.package_by_name(namespace.text(db).as_str()) { | ||
| let defs = package.resolve(db, name, PackageVisibility::Internal); | ||
| if let Some(package) = db.package_by_name(package.text(db).as_str()) { | ||
| let defs = package.resolve(db, name, NamespaceVisibility::Internal); |
There was a problem hiding this comment.
Is this hardcoding of Internal really correct? We could pass through a known visibility from above if we wanted to. I see the doc comment, but it doesn't make sense to me.
| return Vec::new(); | ||
| let defs = match Identifier::classify(db, file, offset) { | ||
| Some(Identifier::Variable { range, .. }) => goto_local_variable(db, file, range), | ||
| Some(Identifier::NamespaceAccess { | ||
| package, | ||
| name, | ||
| visibility, | ||
| }) => goto_namespace_access(db, package, name, visibility), |
There was a problem hiding this comment.
The crux of this PR
| NamespaceAccess { | ||
| namespace: Name<'db>, | ||
| package: Name<'db>, | ||
| name: Name<'db>, | ||
| part: NamespacePart, | ||
| operator_range: TextRange, | ||
| name_range: TextRange, | ||
| visibility: NamespaceVisibility, |
There was a problem hiding this comment.
I have simplified this type for now since nothing actually used part, operator_range, or name_range, and it kind of felt like overkill to keep them around, and it felt like a strange set of things to keep. It seemed reasonable to me that we could add them back as required
DavisVaughan
force-pushed
the
feature/goto-definition-namespaced-sources
branch
from
4ae7f4d to
01f5af3
Compare
2 participants
This is the official version of the POC
feature/sources-demo, with full test coverage and correctNamespaceVisibilityhandling