failure icon indicating copy to clipboard operation
failure copied to clipboard

context() and with_context() are misleading names

Open Alex-PK opened this issue 6 years ago • 7 comments

I think .context() and .with_context() are misleading when reading code using them. They appear to add context to the operation, but they only add information to the error case.

I suggest renaming them to .err_context() and .with_err_context() or something similar to make the code more expressive.

Alex-PK avatar Dec 19 '17 08:12 Alex-PK

I'm open to deprecating context and replacing with a new name that more clearly implies that they are error related. However, I have these constraints:

  1. I don't like using err_ to distinguish things. I think its much better to come up with a natural name than to take the sort of shortcut route of just adding the variant as part of the method name.
  2. I would prefer it to have the same name in ResultExt as it does in Fail.

I think of .context()? as being an operation like .unwrap() and .expect(); that is, a widely understood idiom. So I'd like to find some single word similar to context, but which more clearly implies that its on the error side of things.

withoutboats avatar Mar 01 '18 22:03 withoutboats

I understand the reasoning behind your constraint.

I am not an english native speaker, so I am not sure I can come up with a good suggestion, but I would like to share my point of view regarding the examples you bring.

I like .unwrap() as it's clear that there will be a result in both cases (Ok and Err): i fit was Ok, I get the value; if it was Err I get an error (a panic, to be precise, but nonetheless an error).

OTOH I find .expect() one of the most confusing "keywords" in rust. When I see do_something().expect("something else") I expect (pun intended) that the do_something() will return the value something else, and to get an error if it doesn't. Basically: it's for tests only, and my intuitive guess is wrong. .or_panic(msg) would have been a better, IMO.

The same happens with .context(), in my mind, and that's why I opened the issue.

When I see do_something().context("some text") I think "Ok, this operation is going to use the context I pass for something, like logging or tracking". I absolutely don't think about error wrapping.

With .map_err() or .chain_err() it's extremely clear it's only applied to the error case.

An alternative suggestion could be .or_bail() but I'm not completely sold on this one either, as it's not clear the error is going to be wrapped/linked/contextualised.

I suggested .err_context() because in my mind it made extremely clear what was happening. I am not attached to it.

My only concern is that it should be clear while reading the code what is happening, without having to know the internals of the crate being used.

Alex-PK avatar Mar 05 '18 14:03 Alex-PK

I think Context shouldn't really be called Context but Chain (and as such the names of context and with_context would also change).

I recently changed a not so small crate from a monolithic error_chain to failure using different patterns of errors and error composition depending on each errors usage. My experience was mainly good, but I think Context is bad name, through I know I can understand where the name comes from: you add a "context" in form of e.g. a error kind saying which operation failed to the error.

But most (all?) of this context could be seen as "light wight errors" and as such it can be seen as a form of error chaining.

When I speak about light wight errors I mean errors which have no cause, no backtrace and optimially are copy or at last cheap to clone. If represented as a trait it would be trait LightFail: Display + Send + Sync + 'static {} (+ Clone?) which just happens to be the required constraint for values used as a context in Context.

The problem is that there are many kinds of context's and Context only works for with a very specific kind of context's mainly (not solely) such which you could also see as "light wight errors" like e.g. error kind's &'static str's which can be seen as stringly typed light wight errors etc.

But there are a lot of other context, e.g. a absolute or relative position in a source where the failure originated from, a snapshot of the configuration for the failed operation, etc. While many but not all of them might implement Display. Even this which do should often not be displayed alone, e.g. a message like hy "tom" makes little sense without displaying the error (e.g. NoQuotingAllowed => no quoting allowed, quoting appeard in message: "hy \"tom\"").

This would also resolve the confusion wrt. the utility methods of ResultExt and Fail. I more then once accendentally started typing chain only to correct it to context, because I was thinking about chaining errors (through that might just be me ;=) )

If Context get's renamed to Chain, then .context(kind) could become .chain(kind) and .with_context(|e| kind) could become .chain_with(|e| kind). I will make another post wrt. to the naming.

rustonaut avatar Apr 26 '18 09:04 rustonaut

Expect what I already mentioned about context another think I think could be improved if the name is changed anyway is to not use a with_ prefix.

All places in rust where a function has a variant accepting a value and one accepting a closure it is denoted by a suffix e.g. or => or_else, and => and_then, etc. Sure, context_with/context_then sound a bit stupid and context_from(func) might also not be the best, but if context is changed anyway we might just try to fix this too.

Also all "in-the-wild" use cases of a with_ prefix I stumbled over where functions creating a new value by consuming the value and combining it with another value, but they where not that common an I would have to search for a good example (outside of code written by me, I mean using my own code as example would be pointless right ;) ).

rustonaut avatar Apr 26 '18 09:04 rustonaut

I completely agree with @Alex-PK, both .expect() and .context() are misleading for me. .chain() is still a too broad term since you apply it to the Result which may be either Ok or Err.

Any of the following sound good to me:

  • .map_err() / .map_err_with() (but it is already taken by the standard Result implementation)
  • .chain_err() / .chain_err_with()
  • .wrap_err() / .wrap_err_with() (as opposed to .unwrap())
  • .map_failure() / map_failure_with() (too long?)

I seem to prefer .wrap_err(), but I find all three to express the intention better than the current .context() API.

Other options I have generated:

  • .or_wrap() / .or_wrap_with() (it has some connection to .unwrap(), but it does not reflect the intent clearly to me)
  • .or_failure() / .or_failure_with() (too long and still doesn't sound right to me)

P.S. I noticed that wrap_err may sound like "wrapper", map_err -> "mapper", chain_err -> "chainer", and while it is an unintended pun, it may be still confusing.

frol avatar Apr 26 '18 11:04 frol

@frol I agree that .chain() is a bit to general for the function name, main main concern was to change failure::Context to failure::Chain, which I think isn't to general as for example error are also often called Error instead of something like CrateNameError.

I post the idea to rename the type here instead of another issue as both name changes can be seen as one change in naming.

I don't think a or_ prefix is appropriate as they are used in "if this fail try to use that" manner, e.g. Result.or accept another Result and Option.or accepts another Option.

I also prefer the usage of the word chain over wrap in this context, as you don't just wrap the failure, you create a data structure which is extremely similar to a linked list, it has a "head" (the think currently referred to as the context) and a "tail" (which is Either<Backtrace, Error>). Sure it's not a linked list, as it has backtraces attached, erases the type etc. that's why I call it a Chain (the other reason is, that it would be less confusing for people migrating from error_chain).

So chain_err?

rustonaut avatar Apr 26 '18 12:04 rustonaut

"On" has been used for error handling for 40 years! OnError: ifError

on_error on_error_wrap on_error_report on_error_inform on_error_log on_error_do {}

or perhaps use fail/failure

on_fail on_fail_do {} on_fail_report on_failure_report on_failure_wrap fail_with( a_context )

on_failure wrap_failure_with

keithy avatar Oct 16 '19 22:10 keithy