fix(clap_generate): zsh completion generation panic #2191
Conversation
zsh completion generation would panic if a global argument had conflicts with another argument which was present in its own command but not in its subcommands
the test that was added tests for a panic that would occur when a global argument had a conflict with another argument that wasn't present in all the subcommands that the global argument was present in this would occur inside the get_arg_conflicts_with function
|
This PR would need a MSRV of 1.46.0 Maybe before 1.46.0 one couldn't immutably borrow through indexing? vec.append(&mut self.subcommands[idx].get_subcommands_containing(arg)); Rewriting the patch to work before 1.46.0 would mean a more intrusive change. |
|
Can you please add a TODO regarding 1.46 in the code? |
|
The last commit It was a bit silly of me to forget, that one can get at the items of a vec not only through indexing but also with the According to the CI, the tests now pass on 1.42. |
|
Yes, please add the TODO so that I can upgrade the code later on when moving to 1.46 |
|
I've added a TODO to the code. |
preserving the observable behavior of the existing public api, while handling global arguments separately in get_arg_conflicts_with
|
I have thought about the problem again. |
|
That sounds good to me. |
|
I had already implemented my suggestion of the previous comment at the time of posting. |
| for conflict in conflicts { | ||
| if let Some(s) = conflict.get_short() { | ||
| res.push(format!("-{}", s)); | ||
| match (app_global, arg.get_global()) { |
There was a problem hiding this comment.
Let's decrease the match scope to only get the conflicts. That way you can reduce duplication and don't even need the push_conflicts function.
There was a problem hiding this comment.
Decreasing the scope is problematic because app_global and app have different lifetimes. If I try to handle the conflicts outside the match I get a lifetime mismatch error. Because this isn't performance critical and the duplication isn't that large, I would ask you, if this change is important enough, for me to take a longer look at this again or if the merge can proceed?
|
Can you please do me a favor and check if this fixes #2220? I still can't get zsh working locally. |
|
Yes. I will look into #2220 |
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
Stopped waiting for PR status (Github check) without running due to duplicate requests to run. You may check Bors to see that this PR is included in a batch by one of the other requests. |
|
If you can make a draft PR removing the TODO and changing the code to 1.46, I should get to it sometime next week after upgrading the MSRV. |
|
I can do that. In addition, maybe clap 3 should remove the |
|
Nah. We panic much more earlier in development when this happens. So, removing it here doesn't change much. |
|
I will get the 1.46 support out tomorrow at #2228. Hopefully you can send a PR soon for this TODO item. |
This PR fixes a panic that would occur when a zsh completion file was being generated
for an app that contains global arguments that have conflicts with other arguments which aren't
present in all the subcommands that the global argument would be applied to.
The added test would panic before the fix.
Only the test added without the fix to observe the problem.
git clone --branch zsh_completion_bug https://github.com/ahkrr/clap.git
cd clap/clap_generate/tests && cargo test
I haven't found an open issue regarding this panic.