8 ms·

Would it make sense to have a cargo/rustc flag to disable unwrap and friends when building for production?

by tmzt 10y ago

steveklabnik 10y ago
unwrap has legitimate use-cases, and it's not clear what "disable" it would be, as it changes the type of the thing it returns. You could write a lint to fail the build, if you want, I guess...
- Manishearth 10y ago
  clippy has this already :)
  - bumblebeard 10y ago
    For those not familiar with Clippy: https://github.com/Manishearth/rust-clippy https://github.com/Manishearth/rust-clippy It's a very useful tool; the main thing about it that annoys me is that it only works with nightly.
- tmzt 10y ago
  Right, it would likely use clippy but it would essentially be a --production target or profile, intended for builds where the binary will be run in production. And by 'and friends' I mean calls that panic for the same reason as unwrap, such as expect or ok. The goal is to stop code from reaching production inadvertently, not to prevent all sources of panics.
  - wyldfire 10y ago
    But if we accept the premise that "it's acceptable in some cases to have unwrap() in code that targets 'production'" then it wouldn't make sense to have a production profile that bars its use. The word "production" is in the global namespace and I think you want something more specific to your use case. Rather, one could define a rust coding guide for themselves that deems unwrap() inappropriate for production use. (and in that case use the lint Steve suggests).
    - xorxornop 10y ago
      Seems like you'd want a lint rule where if unwrap is used, it must have a comment preceding it (of some formal syntax) describing why it's necessary or appropriate. Thus the build can have all uses of unwrap known as explicitly allowed, allowing all the possible sites of panics to be enumerated and known, a useful property to have
      - staticassertion 10y ago
        In that case you may want "expect" over unwrap.
- gue5t 10y ago
  .unwrap() is only the right choice if you need to optimize for binary size and can't afford the cost of the precise error message you would pass to .expect(). There are situations where you can't possibly continue running the application if an error occurs, but you shouldn't rely on a backtrace (which you might not manage to capture, e.g. if RUST_BACKTRACE is unset or you don't have symbols) as your only method of communication with your future self.
  - tatterdemalion 10y ago
    This is pedantic. It seems clear from context that this conversation is about panicking on the None/Err cases, and unwrap is a shorthand for "unwrap or except or match with a panic branch."
  - steveklabnik 10y ago
    This is not true. For example, consider this code: if foo.is_some() { let foo = foo.unwrap(); } else { // other code } Here, I _know_ that foo is some. The extra error message from expect will _never_ be seen. Now, this is a contrived example, and would better be written with `if let` in today's Rust, but this is the _kind_ of situation in which unwrap is totally, 100% cool, but the compiler can't know.
    - dep_b 10y ago
      I write Swift daily and I just don't force unwrap anymore, ever. I don't think a hard crash is very usable in a production application, a lot of people disagree and want a hard crash while testing but I think for those bugs that slip through the user experience between for example "loading the first screen but my avatar isn't set" is so much better than "loading the first screen and the app kills itself" just because you force unwrapped the URL of the avatar from the JSON response that had a slight problem in production. Well perhaps we should have something that logs the error in production but keeps on trucking and crashes the application when the debug or test flag is set?
      - steveklabnik 10y ago
conradev 10y ago
If by "and friends" you mean "calls that can panic", there are a few of those. Slice bounds checks, for one.
wyldfire 10y ago
"Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?" Unwrap implies "either this operation should succeed or we should panic". Doing anything other than panic seems like it's terribly difficult to determine what that should be. And I might've put unwrap() there because that's really what I want to happen. For `mv`: don't let's dare go ahead and do the unlink() if the link() failed! Tangent: errors and exceptions aren't bad, they're how computers work. I've encountered folks who, when faced with runtime errors pepper their code with "if (!NULL)" or truly evil things like "except:pass"/"catch (...) { }" which rarely make sense anywhere but the base of the stack and even then don't usually. If you've ever asked yourself "but how did we even get here‽" it may be because someone dropped something totally incongruous like that in a related module.
- Manishearth 10y ago
  I think the idea is to fail to compile if you have certain kinds of panic? I agree that making unwrap/expect silently ... not happen will just cause worse problems.
  - wyldfire 10y ago
    Sure, but in that case it would effectively elide it from the language spec! Who doesn't target "production" eventually?
    - Manishearth 10y ago
      > Sure, but in that case it would effectively elide it from the language spec! No. Some unwraps are necessary. You wouldn't be absolutist here; you could still allow some carefully-labeled unwraps. Also, not all production users need to care about panics that much.
      - tmzt 10y ago
        Yes, I would envision an allow attribute of some sort. This is simply a contract with compiler that you didn't copy and paste some example code somewhere in your codebase that fails with an unwrap. It's about extending the "if it compiles it runs" near-guarantee that we so love about Rust. Ideally a program would fail fast and be restarted if it reached an unrecoverable state, with supervision trees like Erlang. Also ideally, unwrap would be used for exceptional states, not only ones that are unlikely to fail until something goes wrong, like a port being closed or a file unreadble or not present.
burntsushi 10y ago
To put this into perspective, this would also necessarily disable expressions of the form `xs[i]` where `xs` is a slice. Why? Because `xs[i]` is equivalent to `*xs.get(i).unwrap()`. In other words, banning unwrap isn't really that productive because an unwrap, when properly used, is an expression of a runtime invariant. The problem is that unwrap can be very easily misused as an error handling strategy in a library, and in that case, it's pretty much always wrong. But that doesn't mean using unwrap in a library is wrong all on its own, for example.
- kibwen 10y ago
  Sometimes I do wish I could disable the indexing syntax, though. :P At least in my own code, I find that I naturally reach for iterators rather than doing any manual indexing.

Would it make sense to have a cargo/rustc flag to disable unwrap and friends when building for production?

by tmzt 10y ago

Would it make sense to have a cargo/rustc flag to disable unwrap and friends when building for production?

unwrap has legitimate use-cases, and it's not clear what "disable" it would be, as it changes the type of the thing it returns. You could write a lint to fail the build, if you want, I guess...

Manishearth 10y ago
clippy has this already :)
- bumblebeard 10y ago
  For those not familiar with Clippy: https://github.com/Manishearth/rust-clippy https://github.com/Manishearth/rust-clippy It's a very useful tool; the main thing about it that annoys me is that it only works with nightly.
tmzt 10y ago
Right, it would likely use clippy but it would essentially be a --production target or profile, intended for builds where the binary will be run in production. And by 'and friends' I mean calls that panic for the same reason as unwrap, such as expect or ok. The goal is to stop code from reaching production inadvertently, not to prevent all sources of panics.
- wyldfire 10y ago
  But if we accept the premise that "it's acceptable in some cases to have unwrap() in code that targets 'production'" then it wouldn't make sense to have a production profile that bars its use. The word "production" is in the global namespace and I think you want something more specific to your use case. Rather, one could define a rust coding guide for themselves that deems unwrap() inappropriate for production use. (and in that case use the lint Steve suggests).
  - xorxornop 10y ago
    Seems like you'd want a lint rule where if unwrap is used, it must have a comment preceding it (of some formal syntax) describing why it's necessary or appropriate. Thus the build can have all uses of unwrap known as explicitly allowed, allowing all the possible sites of panics to be enumerated and known, a useful property to have
    - staticassertion 10y ago
      In that case you may want "expect" over unwrap.
gue5t 10y ago
.unwrap() is only the right choice if you need to optimize for binary size and can't afford the cost of the precise error message you would pass to .expect(). There are situations where you can't possibly continue running the application if an error occurs, but you shouldn't rely on a backtrace (which you might not manage to capture, e.g. if RUST_BACKTRACE is unset or you don't have symbols) as your only method of communication with your future self.
- tatterdemalion 10y ago
  This is pedantic. It seems clear from context that this conversation is about panicking on the None/Err cases, and unwrap is a shorthand for "unwrap or except or match with a panic branch."
- steveklabnik 10y ago
  This is not true. For example, consider this code: if foo.is_some() { let foo = foo.unwrap(); } else { // other code } Here, I _know_ that foo is some. The extra error message from expect will _never_ be seen. Now, this is a contrived example, and would better be written with `if let` in today's Rust, but this is the _kind_ of situation in which unwrap is totally, 100% cool, but the compiler can't know.
  - dep_b 10y ago
    I write Swift daily and I just don't force unwrap anymore, ever. I don't think a hard crash is very usable in a production application, a lot of people disagree and want a hard crash while testing but I think for those bugs that slip through the user experience between for example "loading the first screen but my avatar isn't set" is so much better than "loading the first screen and the app kills itself" just because you force unwrapped the URL of the avatar from the JSON response that had a slight problem in production. Well perhaps we should have something that logs the error in production but keeps on trucking and crashes the application when the debug or test flag is set?
    - steveklabnik 10y ago

conradev 10y ago

If by "and friends" you mean "calls that can panic", there are a few of those. Slice bounds checks, for one.

wyldfire 10y ago

"Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?" Unwrap implies "either this operation should succeed or we should panic". Doing anything other than panic seems like it's terribly difficult to determine what that should be. And I might've put unwrap() there because that's really what I want to happen. For `mv`: don't let's dare go ahead and do the unlink() if the link() failed! Tangent: errors and exceptions aren't bad, they're how computers work. I've encountered folks who, when faced with runtime errors pepper their code with "if (!NULL)" or truly evil things like "except:pass"/"catch (...) { }" which rarely make sense anywhere but the base of the stack and even then don't usually. If you've ever asked yourself "but how did we even get here‽" it may be because someone dropped something totally incongruous like that in a related module.

Manishearth 10y ago
I think the idea is to fail to compile if you have certain kinds of panic? I agree that making unwrap/expect silently ... not happen will just cause worse problems.
- wyldfire 10y ago
  Sure, but in that case it would effectively elide it from the language spec! Who doesn't target "production" eventually?
  - Manishearth 10y ago
    > Sure, but in that case it would effectively elide it from the language spec! No. Some unwraps are necessary. You wouldn't be absolutist here; you could still allow some carefully-labeled unwraps. Also, not all production users need to care about panics that much.
    - tmzt 10y ago
      Yes, I would envision an allow attribute of some sort. This is simply a contract with compiler that you didn't copy and paste some example code somewhere in your codebase that fails with an unwrap. It's about extending the "if it compiles it runs" near-guarantee that we so love about Rust. Ideally a program would fail fast and be restarted if it reached an unrecoverable state, with supervision trees like Erlang. Also ideally, unwrap would be used for exceptional states, not only ones that are unlikely to fail until something goes wrong, like a port being closed or a file unreadble or not present.

burntsushi 10y ago

To put this into perspective, this would also necessarily disable expressions of the form `xs[i]` where `xs` is a slice. Why? Because `xs[i]` is equivalent to `*xs.get(i).unwrap()`. In other words, banning unwrap isn't really that productive because an unwrap, when properly used, is an expression of a runtime invariant. The problem is that unwrap can be very easily misused as an error handling strategy in a library, and in that case, it's pretty much always wrong. But that doesn't mean using unwrap in a library is wrong all on its own, for example.

kibwen 10y ago
Sometimes I do wish I could disable the indexing syntax, though. :P At least in my own code, I find that I naturally reach for iterators rather than doing any manual indexing.