Track positional and keyword arguments #1459
Conversation
|
Thanks for the PR! This looks pretty good to me but we should get a review from @kinto0 as well |
|
rebased |
ndmitchell
left a comment
ndmitchell
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
stroxler
left a comment
stroxler
left a comment
There was a problem hiding this comment.
I think you rebased past a change in the hover result, hence the test failures
pyrefly/lib/state/lsp.rs
Outdated
| && call.arguments.range.contains_inclusive(find) | ||
| { | ||
| // Check positional arguments | ||
| for (i, arg) in call.arguments.args.as_ref().iter().enumerate() { |
There was a problem hiding this comment.
nit: if this was extracted into a helper function, we wouldn't need the comments since the function name would explain it. it also makes visit_finding_signature_range a lot easier to read
pyrefly/lib/state/lsp.rs
Outdated
| } | ||
| } | ||
| // Check keyword arguments | ||
| let kwarg_start_idx = call.arguments.args.len(); |
There was a problem hiding this comment.
same nit: another helper function? this function is getting big
| enum ActiveArgument { | ||
| Positional(usize), | ||
| Keyword(Name), | ||
| Next(usize), |
There was a problem hiding this comment.
nit: could you add a doc-comment to these? it's unclear what Next is
pyrefly/lib/state/lsp.rs
Outdated
| | Param::VarArg(Some(name), ..) | ||
| | Param::KwOnly(name, ..) | ||
| | Param::Kwargs(Some(name), ..) => Some(name), | ||
| _ => None, |
There was a problem hiding this comment.
looks like this covers all cases so please exclude this wildcard (makes it less bug-prone when we add a new Param)
pyrefly/lib/state/lsp.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn parameter_name(param: &Param) -> Option<&Name> { |
There was a problem hiding this comment.
this function should probably go in pyrefly_types/src/callable.rs as an impl for Param. maybe fmt_with_type should use it?
| mod_module.visit(&mut |x| Self::visit_finding_signature_range(x, position, &mut res)); | ||
| let (callee_range, call_args_range, arg_index) = res?; | ||
| let (callee_range, call_args_range, mut active_argument) = res?; | ||
| if let ActiveArgument::Next(index) = &mut active_argument |
There was a problem hiding this comment.
please add a comment here for what this part does and why it's necessary
stroxler
left a comment
stroxler
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
4 participants
Close #1417.
We need to save the keyword identifier during signature range search to find the correct position when creating signature information.
I added some test cases to
signature_help.rs, and manually verified the fix by building and running LSP locally:There is also a bug in positional arguments, as the newly-added
positional_arguments_testfails on themainbranch. To fix this, we need to count the number of separators (commas) and adjust the index accordingly.