Fix/exec empty arguments crash - #1783
Conversation
|
@SEPURI-SAI-KRISHNA I don't see it crash at runtime. % container ls
Warning! Running debug build. Performance may be degraded.
ID IMAGE OS ARCH STATE IP CPUS MEMORY STARTED
6f6ca580-registry docker-upstream.apple.com/library/registry:3 linux arm64 running 192.168.64.3/24 4 1024 MB 2026-06-23T02:38:21Z
% container exec 6f6ca580-registry
Warning! Running debug build. Performance may be degraded.
Error: Missing expected argument '<arguments> ...'
Help: <arguments> New process arguments
Usage: container exec [<options>] <container-id> <arguments> ...
See 'container exec --help' for more information.That's because swift-argument-parser enforces the invariant The fix you provide is good defensive programming though, and if we were to turn on lint checks at some point we would need to create exceptions for the few force-try expressions scattered through the code. |
|
@jglogan Thanks for the context. You're right that ArgumentParser enforces this before run() is invoked I missed that invariant. Happy to close this if the team prefers keeping the codebase as is. Alternatively, since you mentioned lint checks would flag force-unwraps eventually, I can keep it as a preemptive cleanup but I'll defer to your preference. |
7a23825 to
ac109e9
Compare
|
@SEPURI-SAI-KRISHNA I think the change is fine - you just need to configure commit signing, and perhaps update your Motivation and Context description. |
ac109e9 to
a615d71
Compare
|
@jglogan I have added signature. please check. I have also updated the Motivation and Context. |
a615d71 to
870199c
Compare
|
@SEPURI-SAI-KRISHNA Oops sorry! The signatures have gone while I rebase them on latest main. Could you force push the signed commit again..? |
|
@SEPURI-SAI-KRISHNA Could you rebase this commit to the latest main and force-push the changes? We tried the rebase from the GitHub UI but that dropped the commit signatures. The PR needs to be rebased to pick up an integration test workaround unrelated to your change. Thank you! |
870199c to
7c2e78d
Compare
|
@jglogan @JaewonHur I have rebase and force-pushed. |
Code Coverage
|
|
@SEPURI-SAI-KRISHNA Merged, thanks again for your contribution! |
- `swift-argument-parser` enforces that `arguments` is non-empty before `run()` is invoked, so the force-unwrap of `arguments.first!` is not reachable in practice. However, the guard makes the invariant explicit in the code itself, removes reliance on ArgumentParser's implicit enforcement, and would satisfy force-unwrap lint rules if enabled in the future.
Type of Change
Motivation and Context
swift-argument-parserenforces thatargumentsis non-empty beforerun()is invoked, so the force-unwrap ofarguments.first!is notreachable in practice.
However, the guard makes the invariant explicit in the code itself,
removes reliance on ArgumentParser's implicit enforcement, and would
satisfy force-unwrap lint rules if enabled in the future
Testing