Skip to content
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

print_help() et al. are conceptually read-only ops and should not take &mut self #2218

Closed
djeedai opened this issue Nov 21, 2020 · 3 comments
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies S-duplicate Status: Closed as Duplicate

Comments

@djeedai
Copy link

djeedai commented Nov 21, 2020

print_help() and other similar variants conceptually just print information to some output based on the already-configured App, and therefore it is quite unexpected for them to be mutating the App and take a &mut self reference. For a typical command-line tool which might want to print-help-and-exit deeper inside the program due to other errors not directly handled by clap's argument validation, this forces App to be passed by &mut self everywhere for no good apparent reason, and thereby makes other unrelated parts of the code more complex.

I looked at the implementation, and it seems this is due to some "propagation of globals and settings", which I understand to be lazy operations which need to be done once and could have been done at another time and/or because they are conceptually not mutating the app should be hidden behind a &self read-only reference. I might be wrong here, as I am no clap expert, but I see no good argument from a user standpoint as to why a print function would mutate anything.

Version

  • Rust: rustc 1.48.0 (7eac88abb 2020-11-16)
  • Clap: 2.33.3

Actual Behavior Summary

pub fn print_help(&mut self) -> Result<()>

Expected Behavior Summary

pub fn print_help(&self) -> Result<()>
@djeedai djeedai added the C-bug Category: Updating dependencies label Nov 21, 2020
@pksunkara
Copy link
Member

For better performance. There should be an issue like this already.

@djeedai
Copy link
Author

djeedai commented Nov 21, 2020

I couldn't find any other issue sorry. I've searched for print_help but nothing seems to match.

I understand that how the implementation is lazily doing some ops may be a performance reason, but in my opinion that doesn't warrant making the public interface mutable. Isn't that precisely what interior mutability is for? I can't find the official reference I took that from, but here's another one : https://ricardomartins.cc/2016/06/08/interior-mutability

There are a few general cases that call for interior mutability, such as:
3. Implementation details of logically immutable methods

@epage
Copy link
Member

epage commented Dec 13, 2021

Closing this in favor of #2911 which has more details on the plan going forward.

@epage epage closed this as completed Dec 13, 2021
@epage epage added A-builder Area: Builder API S-duplicate Status: Closed as Duplicate and removed S-waiting-on-decision Status: Waiting on a go/no-go before implementing labels Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies S-duplicate Status: Closed as Duplicate
Projects
None yet
Development

No branches or pull requests

3 participants