-
Notifications
You must be signed in to change notification settings - Fork 88
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
Improve the "goto definition" feature of the LSP by making it work across multiple files #1029
Conversation
fn get_item_kind_with_id(&self, file: FileId, id: ItemId) -> Option<(&ItemId, &TermKind)> { | ||
if file == id.file_id { | ||
// This usage references an item in the file we're currently linearizing | ||
let item = self.linearization.get(id.index)?; | ||
Some((&item.id, &item.kind)) | ||
} else { | ||
// This usage references an item in another file (that has already been linearized) | ||
let item = self | ||
.lin_cache | ||
.get(&id.file_id)? | ||
.get_item(id, self.lin_cache)?; | ||
Some((&item.id, &item.kind)) | ||
} | ||
} | ||
|
||
fn get_item_kind(&self, file: FileId, id: ItemId) -> Option<&TermKind> { | ||
let (_, kind) = self.get_item_kind_with_id(file, id)?; | ||
Some(kind) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have a get_item
and get_item_mut
functions, as in Completed
, rather than those specialized versions? Then the caller could just get the item and decide what to do with it. Here it feels a bit like we are just instantiating a get_item
function with some arbitrary post-processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to rather have a
get_item
andget_item_mut
functions, as inCompleted
, rather than those specialized versions?
That would have been better, but the problem with it is that, with Building
, we're not yet done linearizing the current file, so the linearization items in Building
have the type LinearizationItem<UnifType>
, while the linearization items in the other files would have the type LinearizationItem<Types>
, so that will lead to a return type mismatch from the function. I noticed that we only need the TermKind
(and in some cases, the ItemId
), so this was a nice way to avoid using the whole struct regardless of the generic parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the caller could just get the item and decide what to do with it. Here it feels a bit like we are just instantiating a get_item function with some arbitrary post-processing.
Right. It's just a convenience, because we know the caller does just one thing it. But we can change it to avoid the "post-processing"
let (_, kind) = self.get_item_kind_with_id(file, id)?; | ||
Some(kind) | ||
} | ||
|
||
fn get_item_kind_mut(&mut self, file: FileId, id: ItemId) -> Option<&mut TermKind> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could get rid of this function as well, probably, if having a get_item_mut
.
mut defers: Vec<(ItemId, ItemId, Ident)>, | ||
) { | ||
) -> Vec<(ItemId, ItemId, Ident)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this new return value doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns a list of items that could not be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Then it might be valuable to document this function, and in particular what the return value is.
Co-authored-by: Yann Hamdaoui <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough for get_item_kind_XXX
, if it's used a lot. Do as you see fit, but here is a suggestion though: split the part that fetches the items in separate functions get_item
and get_item_mut
, and use them from within get_item_kind_xxx
. Doing so, we've not added much code (juste moved some parts in dedicated functions), function definitions are smaller, and if we need to get an item for another reason, the methods to do so are already in place.
mut defers: Vec<(ItemId, ItemId, Ident)>, | ||
) { | ||
) -> Vec<(ItemId, ItemId, Ident)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Then it might be valuable to document this function, and in particular what the return value is.
Closes #932
This change adds support for cross file navigation using the "goto reference" feature of the LSP.