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

Decide whether to keep conditions #9795

Closed
brson opened this issue Oct 10, 2013 · 11 comments · Fixed by #12039
Closed

Decide whether to keep conditions #9795

brson opened this issue Oct 10, 2013 · 11 comments · Fixed by #12039
Milestone

Comments

@brson
Copy link
Contributor

brson commented Oct 10, 2013

It's not clear that we want to commit to conditions - they haven't proved effective yet and we may be 'biting off more than we can chew' by embedding them in the std APIs.

Nominating.

@thestinger
Copy link
Contributor

I think in cases where functions are called to perform a side-effect like creating a directory, we could have an RAII-based Expected<T> type throwing a failure in the destructor if the error was not handled (if we truly support failing inside destructors). To handle or ignore the error, the caller would call a method to convert it into a bool or the T type - disabling the destructor. This would depend on temporaries being destroyed immediately (or at the end of the expression) rather than at the end of a block.

http://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2012-Andrei-Alexandrescu-Systematic-Error-Handling-in-C

I find the use of conditions makes handling the error too painful, and doesn't make sense when the function can't actually recover internally. It might make sense to just represent recovery with an Option<&fn()> parameter in some cases rather than conditions.

From what I can see, conditions would be very useful for improving error handling in an existing large application, but not so much in the tiny call stacks of the standard library.

@alexcrichton
Copy link
Member

One thing that I've found is that this suffers from one of the things I don't like about exceptions in that it's easy to have an array of conditions and forget which ones need to get handlers registered and then which ones to raise on. The runtime's I/O currently has a read_error and an io_error condition, and at least from the names it's not entirely clear what the distinction is. They're also inconsistently raised on I believe throughout the implementations because there's no guarantee that it's raised on when it needs to be.

On top of that, I've always found the syntax a little odd for something that one would expect to be dealing with frequently. Declaring the "exception handler" before the "exceptionable block" always seemed a little odd to me syntactically.

That being said, I do think that they still have good uses in a few cases, but I agree with @thestinger that for the standard library with very small call stacks back to the original invoker, I would rather see some sort of return value propagated upwards instead of a condition being raised on. I'm personally ok with Result, but I know many others have had opposition to it. I'd be interested to explore the idea of failing if you didn't handle the error, it may end up causing code to be too noisy, but it could also help expose some very real bugs.

@catamorphism
Copy link
Contributor

I like that conditions can be used to implement fail-safe behavior without lots of explicit result-checking and unwrapping. I don't like the fact that "replace the erroneous value with a new one, then return control to the callee" is a high priority when in my own work, I've never run into a scenario when that actually happens. It just seems to make things awkward. Maybe I'm doin' it wrong, but if so, that's a documentation issue.

One alternative is to implement a special case of monadic "do" notation (of course we wouldn't use do) for the Result and Option monads. I think in the common case, that would be a lot less painful than the status quo.

@nikomatsakis
Copy link
Contributor

I've personally found results quite nice for propagating errors backwards. I am not opposed to conditions but I confess I do not fully see how they work either.

Do notation can be implemented with a macro easily enough. However, I've personally found that the if_ok! macro used in the infer code (and a few other places) is perfect:

macro_rules! if_ok(
    ($inp: expr) => (
        match $inp {
            Ok(v) => { v }
            Err(e) => { return Err(e); }
        }
    )
)

Basically this takes a Result<T,E> expression and propagates the error, hence the resulting type is just T. This can then be used like so:

        let tps = if_ok!(self.tps(as_.tps, bs.tps));
        let self_ty = if_ok!(self.self_tys(as_.self_ty, bs.self_ty));
        let regions = if_ok!(relate_region_params(self,
                                                  item_def_id,
                                                  &as_.regions,
                                                  &bs.regions));
        Ok(substs { regions: regions,
                    self_ty: self_ty,
                    tps: tps.clone() })

I would not object to some sort of macro for dealing with results being part of the standard library.

@brson
Copy link
Contributor Author

brson commented Oct 16, 2013

Let's remove conditions.

@catamorphism
Copy link
Contributor

I'm ok with that, but I think we should add the macros that Niko suggested to the standard library before removing conditions.

@catamorphism
Copy link
Contributor

1.0, back-compat

@Florob
Copy link
Contributor

Florob commented Oct 24, 2013

FWIW, I actually like conditions a lot for cases where you can handle the error and continue. I currently use them in my XML library when an entity is not recognized. It allows users to catch e.g. © not being defined and passing back a ©. I wouldn't know how to do this nicely without conditions.

@huonw
Copy link
Member

huonw commented Oct 24, 2013

@Florob, except for the condition! macro, conditions are actually defined entirely in std::condition, so you should be able to adapt the functionality you need.

@Florob
Copy link
Contributor

Florob commented Oct 24, 2013

@huonw, Yes I will certainly survive if this feature is removed.
My point is, there are cases where conditions are generally useful, and others (like opening files) where they are not. Just like the Condition and Error-handling Tutorial actually suggests.

I would agree that conditions are currently not properly utilized in the stdlib though. From a first look a somewhat sensible case could be the str::not_utf8 condition, but it would be far more useful if the handler got the index of the first bad byte as an integer value, and not embedded in some error string. It would also be possible to allow replacing a sequence of bad bytes, instead of having to return a replacement for the whole string. The beauty of conditions, to me, really is that you can call into the handler more than once.

@thestinger
Copy link
Contributor

I would prefer using an explicit handler function parameter when the condition paradigm is needed. The TLS implementation allows adding conditions without changing the external API, but I consider that to be a bad thing...

bors added a commit that referenced this issue Feb 7, 2014
This has been a long time coming. Conditions in rust were initially envisioned
as being a good alternative to error code return pattern. The idea is that all
errors are fatal-by-default, and you can opt-in to handling the error by
registering an error handler.

While sounding nice, conditions ended up having some unforseen shortcomings:

* Actually handling an error has some very awkward syntax:

        let mut result = None;                                        
        let mut answer = None;                                        
        io::io_error::cond.trap(|e| { result = Some(e) }).inside(|| { 
            answer = Some(some_io_operation());                       
        });                                                           
        match result {                                                
            Some(err) => { /* hit an I/O error */ }                   
            None => {                                                 
                let answer = answer.unwrap();                         
                /* deal with the result of I/O */                     
            }                                                         
        }                                                             

  This pattern can certainly use functions like io::result, but at its core
  actually handling conditions is fairly difficult

* The "zero value" of a function is often confusing. One of the main ideas
  behind using conditions was to change the signature of I/O functions. Instead
  of read_be_u32() returning a result, it returned a u32. Errors were notified
  via a condition, and if you caught the condition you understood that the "zero
  value" returned is actually a garbage value. These zero values are often
  difficult to understand, however.

  One case of this is the read_bytes() function. The function takes an integer
  length of the amount of bytes to read, and returns an array of that size. The
  array may actually be shorter, however, if an error occurred.

  Another case is fs::stat(). The theoretical "zero value" is a blank stat
  struct, but it's a little awkward to create and return a zero'd out stat
  struct on a call to stat().

  In general, the return value of functions that can raise error are much more
  natural when using a Result as opposed to an always-usable zero-value.

* Conditions impose a necessary runtime requirement on *all* I/O. In theory I/O
  is as simple as calling read() and write(), but using conditions imposed the
  restriction that a rust local task was required if you wanted to catch errors
  with I/O. While certainly an surmountable difficulty, this was always a bit of
  a thorn in the side of conditions.

* Functions raising conditions are not always clear that they are raising
  conditions. This suffers a similar problem to exceptions where you don't
  actually know whether a function raises a condition or not. The documentation
  likely explains, but if someone retroactively adds a condition to a function
  there's nothing forcing upstream users to acknowledge a new point of task
  failure.

* Libaries using I/O are not guaranteed to correctly raise on conditions when an
  error occurs. In developing various I/O libraries, it's much easier to just
  return `None` from a read rather than raising an error. The silent contract of
  "don't raise on EOF" was a little difficult to understand and threw a wrench
  into the answer of the question "when do I raise a condition?"

Many of these difficulties can be overcome through documentation, examples, and
general practice. In the end, all of these difficulties added together ended up
being too overwhelming and improving various aspects didn't end up helping that
much.

A result-based I/O error handling strategy also has shortcomings, but the
cognitive burden is much smaller. The tooling necessary to make this strategy as
usable as conditions were is much smaller than the tooling necessary for
conditions.

Perhaps conditions may manifest themselves as a future entity, but for now
we're going to remove them from the standard library.

Closes #9795
Closes #8968
flip1995 pushed a commit to flip1995/rust that referenced this issue Nov 21, 2022
Add a config, allow-print-in-tests, that can be set in clippy.toml which
allows the usage of `[e]print[ln]!` macros in tests.

Closes rust-lang#9795
flip1995 pushed a commit to flip1995/rust that referenced this issue Nov 21, 2022
Add allow-print-in-tests config

Add a config, allow-print-in-tests, that can be set in clippy.toml which allows the usage of `[e]print[ln]!` macros in tests.

Closes rust-lang#9795

---

changelog: Enhancement: [print_stdout], [print_stderr]: Can now be enabled in test with the `allow-print-in-tests` config value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants